Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14923004:
[Mac] AppController needs to update its "last profile" pointer when the active profile is deleted (Closed)
|
Created:
7 years, 7 months ago by noms (inactive) Modified:
7 years, 5 months ago CC:
chromium-reviews, sail+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[Mac] AppController needs to update its "last profile" pointer when the active profile is deleted.
Otherwise, when the next browser window is opened, it will try to use the old
"last used" profile, which is the one that just got deleted. This also means that
the ProfileManager needs to pre-load the next active profile, if one is not loaded
already.
BUG=194415
TEST=On Mac, create two profiles. Ensure only the active profile has opened browser windows. Delete the active profile. All windows should close, but the Chrome icon should still be highlighted in the dock. Clicking it should not crash, and should open a window for the remaining profile.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209035
Patch Set 1 #
Total comments: 4
Patch Set 2 : Refactored #Patch Set 3 : Fixed style #
Total comments: 7
Patch Set 4 : Virtual functions should be marked virtual #
Total comments: 6
Patch Set 5 : checkpoint #Patch Set 6 : Fix race condition and made code awesomer #Patch Set 7 : Fix whitespace #
Total comments: 14
Patch Set 8 : Drive-by fixes #
Total comments: 6
Patch Set 9 : deal with multiple simultaneous deletions #
Total comments: 18
Patch Set 10 : yay, unit tests!! #Patch Set 11 : rebase & fixed comments #
Total comments: 27
Patch Set 12 : refactored tests #Patch Set 13 : rebase #
Total comments: 9
Patch Set 14 : fix recursive call and add test for it #
Total comments: 8
Patch Set 15 : nits #Patch Set 16 : nits #
Total comments: 12
Patch Set 17 : fix broken windows code #
Total comments: 16
Patch Set 18 : last nits #Patch Set 19 : rebase #Patch Set 20 : fix import order after rebase #Patch Set 21 : fix android test #Patch Set 22 : derp. no test on CrOS either. #
Messages
Total messages: 49 (0 generated)
Hi Alexei, Here's a first draft of the fix for the profile deletion bug. Would you mind giving it a look-over? I'm scared I used all the wrong Objective-C things :)
Since the C++ class is so small, you can just put it at the top of the same file (chrome/browser/app_controller_mac.mm), and forward declare it in the header. See bookmark_context_menu_cocoa_controller.mm for an example.
https://codereview.chromium.org/14923004/diff/1/chrome/browser/app_controller... File chrome/browser/app_controller_profile_observer.h (right): https://codereview.chromium.org/14923004/diff/1/chrome/browser/app_controller... chrome/browser/app_controller_profile_observer.h:32: }; I think you need DISALLOW_COPY_AND_ASSIGN() here. https://codereview.chromium.org/14923004/diff/1/chrome/browser/app_controller... File chrome/browser/app_controller_profile_observer.mm (right): https://codereview.chromium.org/14923004/diff/1/chrome/browser/app_controller... chrome/browser/app_controller_profile_observer.mm:14: if (profile_manager_) { add DCHECK(profile_manager_) ?
https://codereview.chromium.org/14923004/diff/1/chrome/browser/app_controller... File chrome/browser/app_controller_profile_observer.h (right): https://codereview.chromium.org/14923004/diff/1/chrome/browser/app_controller... chrome/browser/app_controller_profile_observer.h:32: }; On 2013/05/07 20:54:15, Roger Tawa wrote: > I think you need DISALLOW_COPY_AND_ASSIGN() here. Done. https://codereview.chromium.org/14923004/diff/1/chrome/browser/app_controller... File chrome/browser/app_controller_profile_observer.mm (right): https://codereview.chromium.org/14923004/diff/1/chrome/browser/app_controller... chrome/browser/app_controller_profile_observer.mm:14: if (profile_manager_) { On 2013/05/07 20:54:15, Roger Tawa wrote: > add DCHECK(profile_manager_) ? Done.
https://codereview.chromium.org/14923004/diff/10001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/14923004/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:204: profile_manager_->GetProfileInfoCache().AddObserver(this); I don't think you need the DCHECK() if the following line would crash already if it was NULL. The point of the DCHECK() is to catch errors earlier than the bad stuff they would cause to happen. https://codereview.chromium.org/14923004/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:213: ProfileManager* profile_manager_; These should be below the methods. https://codereview.chromium.org/14923004/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:219: void OnProfileAdded(const base::FilePath& profile_path) { All of these should be marked "virtual" and "OVERRIDE".
https://codereview.chromium.org/14923004/diff/10001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/14923004/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:204: profile_manager_->GetProfileInfoCache().AddObserver(this); I originally had an if (profile_manager_) check, and Roger suggested I should replace it with a DCHECK(). Do you think there should be no check at all, or no DCHECK() but with an if () check? On 2013/05/08 15:25:06, Alexei Svitkine wrote: > I don't think you need the DCHECK() if the following line would crash already if > it was NULL. The point of the DCHECK() is to catch errors earlier than the bad > stuff they would cause to happen.
https://codereview.chromium.org/14923004/diff/10001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/14923004/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:204: profile_manager_->GetProfileInfoCache().AddObserver(this); On 2013/05/08 15:56:53, Monica Dinculescu wrote: > I originally had an if (profile_manager_) check, and Roger suggested I should > replace it with a DCHECK(). Do you think there should be no check at all, or no > DCHECK() but with an if () check? Don't have an if check unless there's a scenario where the profile_manager_ can actually be NULL (e.g. maybe in some tests?). If it is possible in tests, then you need the if (or somehow make the tests ensure that it won't be NULL).
https://codereview.chromium.org/14923004/diff/10001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/14923004/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:213: ProfileManager* profile_manager_; On 2013/05/08 15:25:06, Alexei Svitkine wrote: > These should be below the methods. Done. https://codereview.chromium.org/14923004/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:219: void OnProfileAdded(const base::FilePath& profile_path) { On 2013/05/08 15:25:06, Alexei Svitkine wrote: > All of these should be marked "virtual" and "OVERRIDE". Done.
https://codereview.chromium.org/14923004/diff/15001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/14923004/diff/15001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:536: g_browser_process->profile_manager()->GetLastUsedProfile()]; Wouldn't this still be a problem? i.e. I think calling GetLastUserProfile() here in the scenario you're trying to fix will cause IO to be done on the UI thread, as the profile manager tries to load the profile. Or does g_browser_process->profile_manager()->GetLastUsedProfile() return NULL in this case? https://codereview.chromium.org/14923004/diff/15001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:774: string16 lastProfileName = UTF8ToUTF16(lastProfile->GetPrefs()->GetString( Instead of calling stuff on a profile that may be the one that's being removed, can you cache the profile_path of the last profile when it gets set, so that you can then simply compare with the profile_path passed to OnProfileWasRemoved()? https://codereview.chromium.org/14923004/diff/15001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:781: lastProfile_ = NULL; Wouldn't all the state that's set up by -windowChangedToProfile: be invalid now?
https://codereview.chromium.org/14923004/diff/15001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/14923004/diff/15001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:536: g_browser_process->profile_manager()->GetLastUsedProfile()]; And how! However, fixed. On 2013/05/08 19:19:11, Alexei Svitkine wrote: > Wouldn't this still be a problem? i.e. I think calling GetLastUserProfile() here > in the scenario you're trying to fix will cause IO to be done on the UI thread, > as the profile manager tries to load the profile. > > Or does g_browser_process->profile_manager()->GetLastUsedProfile() return NULL > in this case? https://codereview.chromium.org/14923004/diff/15001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:774: string16 lastProfileName = UTF8ToUTF16(lastProfile->GetPrefs()->GetString( Ideally, lastProfile_ and lastProfilePath_ would be in sync, but since we can't really control where lastProfile_ might be updated (it's just a member variable), it's kind of risky to hope lastProfilePath_ might be updated at the same time. Also, it doesn't matter that the profile is the one being deleted, as the profile instances don't get deleted (there's a todo in ProfileManager) :( On 2013/05/08 19:19:11, Alexei Svitkine wrote: > Instead of calling stuff on a profile that may be the one that's being removed, > can you cache the profile_path of the last profile when it gets set, so that you > can then simply compare with the profile_path passed to OnProfileWasRemoved()? https://codereview.chromium.org/14923004/diff/15001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:781: lastProfile_ = NULL; On 2013/05/08 19:19:11, Alexei Svitkine wrote: > Wouldn't all the state that's set up by -windowChangedToProfile: be invalid now? Done.
Drive by comments. https://codereview.chromium.org/14923004/diff/25001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.h (right): https://codereview.chromium.org/14923004/diff/25001/chrome/browser/app_contro... chrome/browser/app_controller_mac.h:43: BOOL lastProfileWasDeleted; Please add trailing _ and comment. https://codereview.chromium.org/14923004/diff/25001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/14923004/diff/25001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:203: DCHECK(profile_manager_); Add dcheck for app_controller_ too? From code seems incorrect for caller to pass null. https://codereview.chromium.org/14923004/diff/25001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:571: lastProfileWasDeleted = NO; Is there a ctor where this is initialized? https://codereview.chromium.org/14923004/diff/25001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1180: } Don't need { and }. https://codereview.chromium.org/14923004/diff/25001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/14923004/diff/25001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1113: #if defined(OS_MACOSX) # should be at start of line. https://codereview.chromium.org/14923004/diff/25001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1120: profile_dir), Why does FinishDeletingProfile() need to be deferred in this case? Its not deferred when creating a profile at line 1100 above. https://codereview.chromium.org/14923004/diff/25001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1138: } Don't need { and }.
https://codereview.chromium.org/14923004/diff/25001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/14923004/diff/25001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:571: lastProfileWasDeleted = NO; There is no constructor per se, but this is initialized in applicationDidFinishLaunching(), at the same time as the profileInfoCacheObserver_ is initialized. On 2013/05/14 14:38:40, Roger Tawa wrote: > Is there a ctor where this is initialized? https://codereview.chromium.org/14923004/diff/25001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/14923004/diff/25001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1120: profile_dir), Above, we're deleting the only profile in the list, and the callback OnOpenWindowForNewProfile actually re-opens a new browser window for the new active profile. In this case, we're deleting one of the profiles, but on the mac we need to pre-load the next one. The deferred callback is the one that closes the browser windows for the current profile (among other things), so by chaining them we can make sure that when the user does need a new active profile (after this one has closed), we have a new one ready. On 2013/05/14 14:38:40, Roger Tawa wrote: > Why does FinishDeletingProfile() need to be deferred in this case? Its not > deferred when creating a profile at line 1100 above.
https://codereview.chromium.org/14923004/diff/25001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.h (right): https://codereview.chromium.org/14923004/diff/25001/chrome/browser/app_contro... chrome/browser/app_controller_mac.h:43: BOOL lastProfileWasDeleted; On 2013/05/14 14:38:40, Roger Tawa wrote: > Please add trailing _ and comment. Done. https://codereview.chromium.org/14923004/diff/25001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/14923004/diff/25001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:203: DCHECK(profile_manager_); On 2013/05/14 14:38:40, Roger Tawa wrote: > Add dcheck for app_controller_ too? From code seems incorrect for caller to pass > null. Done. https://codereview.chromium.org/14923004/diff/25001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1180: } On 2013/05/14 14:38:40, Roger Tawa wrote: > Don't need { and }. Done. https://codereview.chromium.org/14923004/diff/25001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/14923004/diff/25001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1113: #if defined(OS_MACOSX) On 2013/05/14 14:38:40, Roger Tawa wrote: > # should be at start of line. Done. https://codereview.chromium.org/14923004/diff/25001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1138: } On 2013/05/14 14:38:40, Roger Tawa wrote: > Don't need { and }. Done.
I think this is on the right track. https://codereview.chromium.org/14923004/diff/31001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/14923004/diff/31001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1071: bool new_last_profile_being_loaded = false; Instead of keeping track of this here, just do an early return in the mac-specific code. https://codereview.chromium.org/14923004/diff/31001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1116: last_non_managed_profile_index); If you're just going to query the path of that profile again, why not save off the path rather than the index of the profile in the loop? (Since the loop code is already querying the path.) You can then just keep the path and compute |last_non_managed_profile| from the path in the if above. https://codereview.chromium.org/14923004/diff/31001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1118: base::Bind(&ProfileManager::OnNewActiveProfileLoaded, Hmm, this may be tricky. I think you should loop in a profiles owner who touched this code on the review. Essentially, you're now leaving the UI thread to perform some stuff on the IO thread while this profile is in the middle of being deleted. So there may be some inconsistent state now while this is happening. There's a potential race where something else may happen on the UI thread that may interact with profile manager while the async task is in progress, before this comes back to the UI thread. The good news is that it looks like this codepath currently doesn't change anything except setting prefs::kProfileLastUsed, so there's not much inconsistent state to cause problems. On the other, setting the pref before the profile gets loaded still causes a problem. So you should do it in the callback (after that profile has loaded), else the race is someone using last_non_managed_profile before the callback runs. The other case you need to worry about is if there's multiple deletions happening in sequence, so while you went off to load last_non_managed_profile on another thread, what happens when a request to delete that profile comes in on the UI thread? (It may already be handled well, but please make sure to reason about this to ensure you understand what happens and why it's okay, and add it to a comment.) Also, this could use some tests.
+ Jeremy Jeremy, do you have any insights on whether this is the right way to go about it (more specifically about Alexei's comment on the last patch)?
Hi Monica, First off, thank you so much for tackling this and finding the root cause!! Profile deletion is very delicate and these bugs can be quite involved, so massive kudos! I touched this code quite a while ago so my recollection is quite fuzzy, please take everything I say with a grain of salt and accept my apologies if I'm misunderstanding the issue. +1 on Alexei's remark about tests - this stuff is really delicate and I think it's important to have a unit test for this if at all possible. Are there any other places in the code where objects hold a pointer to a ProfileManager? The real problem here seems to be that app_controller_mac has a different lifetime from the ProfileManager and can get a stale pointer but it seems this could happen in other places in the code, so I wonder if we need a more general solution... Would it be possible to change all places that hold pointers to the ProfileManager to use WeakPtr's?
Oh, sorry! Miranda had cc'ed you on the bug, so I thought you'd be the source of truth here. :) Do you know who else might be a good resource? Now, to answer your comment, I think the profile_manager lifetime is a bit of a red herring. The pointer is fine, and all the individual Profile* pointers are also ok (when you delete a profile, the pointer isn't actually invalidated, so that's pretty forgiving too), and they are in the ProfileInfoCache as they should be. It's just that the Profile the app_controller_mac would like to use is not loaded. Profiles are only loaded if they're needed -- for example when the user uses the avatar menu to switch to a different profile. In this case, the app_controller_mac is expecting a profile to be loaded, but because the user hasn't specifically asked for it before, it isn't ready and things blow up. In the meantime, I'll get working on the tests so that at least we can reliably test the fix. On 2013/05/16 11:47:41, jeremy wrote: > Hi Monica, > > First off, thank you so much for tackling this and finding the root cause!! > Profile deletion is very delicate and these bugs can be quite involved, so > massive kudos! > > I touched this code quite a while ago so my recollection is quite fuzzy, please > take everything I say with a grain of salt and accept my apologies if I'm > misunderstanding the issue. > > +1 on Alexei's remark about tests - this stuff is really delicate and I think > it's important to have a unit test for this if at all possible. > > Are there any other places in the code where objects hold a pointer to a > ProfileManager? The real problem here seems to be that app_controller_mac has a > different lifetime from the ProfileManager and can get a stale pointer but it > seems this could happen in other places in the code, so I wonder if we need a > more general solution... > > Would it be possible to change all places that hold pointers to the > ProfileManager to use WeakPtr's?
On Thu, May 16, 2013 at 5:56 PM, <noms@chromium.org> wrote: > Oh, sorry! Miranda had cc'ed you on the bug, so I thought you'd be the > source of > truth here. :) Do you know who else might be a good resource? > I worked on some corner cases around deleting profiles, but it's been a while... > Now, to answer your comment, I think the profile_manager lifetime is a bit > of a > red herring. The pointer is fine, and all the individual Profile* pointers > are > also ok (when you delete a profile, the pointer isn't actually > invalidated, so > that's pretty forgiving too), and they are in the ProfileInfoCache as they > should be. > That's exactly the part that scares me - the fact we don't actually delete profiles before shutdown is something we need to be careful of. IMHO using a pattern where bad behavior becomes visible bad or impossible is the way to go. I think using an OwnPtr and explicitly invalidating users or some other patter where it's very clear in the code that a profile has gone away is the right way to go about fixing this. Otherwise we're just playing wack-a-mole with these bugs which are sadly all-too-common.
https://codereview.chromium.org/14923004/diff/31001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/14923004/diff/31001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1071: bool new_last_profile_being_loaded = false; On 2013/05/14 15:34:12, Alexei Svitkine wrote: > Instead of keeping track of this here, just do an early return in the > mac-specific code. Done. https://codereview.chromium.org/14923004/diff/31001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1116: last_non_managed_profile_index); On 2013/05/14 15:34:12, Alexei Svitkine wrote: > If you're just going to query the path of that profile again, why not save off > the path rather than the index of the profile in the loop? (Since the loop code > is already querying the path.) > > You can then just keep the path and compute |last_non_managed_profile| from the > path in the if above. Done. https://codereview.chromium.org/14923004/diff/31001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1118: base::Bind(&ProfileManager::OnNewActiveProfileLoaded, Done most of the comments. Working on the tests. On 2013/05/14 15:34:12, Alexei Svitkine wrote: > Hmm, this may be tricky. I think you should loop in a profiles owner who touched > this code on the review. > > Essentially, you're now leaving the UI thread to perform some stuff on the IO > thread while this profile is in the middle of being deleted. So there may be > some inconsistent state now while this is happening. > > There's a potential race where something else may happen on the UI thread that > may interact with profile manager while the async task is in progress, before > this comes back to the UI thread. > > The good news is that it looks like this codepath currently doesn't change > anything except setting prefs::kProfileLastUsed, so there's not much > inconsistent state to cause problems. > > On the other, setting the pref before the profile gets loaded still causes a > problem. So you should do it in the callback (after that profile has loaded), > else the race is someone using last_non_managed_profile before the callback > runs. > > The other case you need to worry about is if there's multiple deletions > happening in sequence, so while you went off to load last_non_managed_profile > on another thread, what happens when a request to delete that profile comes in > on the UI thread? (It may already be handled well, but please make sure to > reason about this to ensure you understand what happens and why it's okay, and > add it to a comment.) > > Also, this could use some tests.
Comments your way. Still waiting for a test! https://codereview.chromium.org/14923004/diff/43001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/14923004/diff/43001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:786: lastProfileWasDeleted_ = YES; Instead of introducing this variable to track this, can't you just update lastProfile_ here? (via g_browser_process->profile_manager()->GetLastUsedProfile()) With your changes to profile_manager.cc, it should be loaded already, right? https://codereview.chromium.org/14923004/diff/43001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/14923004/diff/43001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1089: std::string last_non_managed_profile = Nit: Make it const https://codereview.chromium.org/14923004/diff/43001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1123: // For OS_MACOSX we will update the local_state in the callback Nit: "update the local_state" -> "update the pref" Also, don't use "we" in the comment. https://codereview.chromium.org/14923004/diff/43001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1132: void ProfileManager::OnNewActiveProfileLoaded(const base::FilePath& profile_dir, This param should be on the next line, aligned with the others. Also, rename it to |profile_to_delete_dir|, for clarity. https://codereview.chromium.org/14923004/diff/43001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1133: const std::string last_non_managed_profile, Should be a "const std::string&". https://codereview.chromium.org/14923004/diff/43001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1135: Profile* profile, Rename to |loaded_profile|. https://codereview.chromium.org/14923004/diff/43001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1142: } else if (status == Profile::CREATE_STATUS_FAIL) { Hmm, I see CREATE_STATUS_LOCAL_FAIL and CREATE_STATUS_REMOTE_FAIL. Has the code changed under you? https://codereview.chromium.org/14923004/diff/43001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1143: // If we are here, it means that the profile we tried to load as the Nit: No "we"'s https://codereview.chromium.org/14923004/diff/43001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/14923004/diff/43001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.h:318: void OnNewActiveProfileLoaded(const base::FilePath& profile_dir, Document the params in a comment.
https://codereview.chromium.org/14923004/diff/43001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/14923004/diff/43001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:786: lastProfileWasDeleted_ = YES; yup, you're right. Done. On 2013/06/05 20:00:52, Alexei Svitkine wrote: > Instead of introducing this variable to track this, can't you just update > lastProfile_ here? (via > g_browser_process->profile_manager()->GetLastUsedProfile()) > > With your changes to profile_manager.cc, it should be loaded already, right? https://codereview.chromium.org/14923004/diff/43001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/14923004/diff/43001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1089: std::string last_non_managed_profile = On 2013/06/05 20:00:52, Alexei Svitkine wrote: > Nit: Make it const Done. https://codereview.chromium.org/14923004/diff/43001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1123: // For OS_MACOSX we will update the local_state in the callback On 2013/06/05 20:00:52, Alexei Svitkine wrote: > Nit: "update the local_state" -> "update the pref" > > Also, don't use "we" in the comment. Done. https://codereview.chromium.org/14923004/diff/43001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1132: void ProfileManager::OnNewActiveProfileLoaded(const base::FilePath& profile_dir, On 2013/06/05 20:00:52, Alexei Svitkine wrote: > This param should be on the next line, aligned with the others. Also, rename it > to |profile_to_delete_dir|, for clarity. Done. https://codereview.chromium.org/14923004/diff/43001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1133: const std::string last_non_managed_profile, On 2013/06/05 20:00:52, Alexei Svitkine wrote: > Should be a "const std::string&". Done. https://codereview.chromium.org/14923004/diff/43001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1135: Profile* profile, On 2013/06/05 20:00:52, Alexei Svitkine wrote: > Rename to |loaded_profile|. Done. https://codereview.chromium.org/14923004/diff/43001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1142: } else if (status == Profile::CREATE_STATUS_FAIL) { And how! On 2013/06/05 20:00:52, Alexei Svitkine wrote: > Hmm, I see CREATE_STATUS_LOCAL_FAIL and CREATE_STATUS_REMOTE_FAIL. Has the code > changed under you? https://codereview.chromium.org/14923004/diff/43001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1143: // If we are here, it means that the profile we tried to load as the On 2013/06/05 20:00:52, Alexei Svitkine wrote: > Nit: No "we"'s Done. https://codereview.chromium.org/14923004/diff/43001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/14923004/diff/43001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.h:318: void OnNewActiveProfileLoaded(const base::FilePath& profile_dir, On 2013/06/05 20:00:52, Alexei Svitkine wrote: > Document the params in a comment. Done.
Hi Sailesh, I made some changes to profile deletion code in the ProfileManager. Would you mind taking a look, please? Thanks!
I still need to look at the new tests to properly review them, but here's some comments I already entered. https://codereview.chromium.org/14923004/diff/57001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/14923004/diff/57001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:782: if (profileName == lastProfileName) { Nit: No {}'s https://codereview.chromium.org/14923004/diff/57001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1174: Profile* lastUsedProfile = Nit: Just return it directly. https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:169: return; Nit: Indention looks wrong. https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:171: if (!is_mounted) What's this dangling if? https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:400: continue; Nit: Indention looks wrong and inner loop shouldn't have {}'s if its a single line. By the way, did you actually mean to remove these LOG statements? https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1126: BrowserList::CloseAllBrowsersWithProfile(profile); Nit:Indention is wrong.
https://codereview.chromium.org/14923004/diff/57001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/14923004/diff/57001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:223: [app_controller_ profileWasRemoved:profile_name]; It would be more direct to use profile_path to reference the deleted profile. https://codereview.chromium.org/14923004/diff/57001/chrome/browser/app_contro... File chrome/browser/app_controller_mac_unittest.mm (right): https://codereview.chromium.org/14923004/diff/57001/chrome/browser/app_contro... chrome/browser/app_controller_mac_unittest.mm:21: class ProfileRemovalObserver : public ProfileInfoCacheObserver { new line above https://codereview.chromium.org/14923004/diff/57001/chrome/browser/app_contro... chrome/browser/app_controller_mac_unittest.mm:35: private: new line above https://codereview.chromium.org/14923004/diff/57001/chrome/browser/app_contro... chrome/browser/app_controller_mac_unittest.mm:94: ProfileRemovalObserver observer(manager->profile_manager(), do you need this observer? Could you just do a check below the ScheduleProfileForDeletion() call? https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1075: // On the Mac, the browser process is not killed when all browser windows Could we just implement this on all platforms?
And some more comments your way... https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_manager_unittest.cc (right): https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager_unittest.cc:502: ProfileInfoCache& cache = profile_manager->GetProfileInfoCache(); Either make it const or a pointer. Style guide disallows non-const references. (And yes, I know ProfileInfoCache is used like that from many places.) https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager_unittest.cc:517: string16(), string16(), false); Can you refactor this block into a helper function that just takes dest_path1 and mock_observer, so the test is more readable? https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager_unittest.cc:534: base::Bind(&CheckLastUsedProfile, dest_path2, "New Profile 2")); Do you need this class? Can't you just do the calls below (schedule deletion, run until idle) and then assert that the profile was deleted and then check last used profile? https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager_unittest.cc:590: } Can you also add a test that exercises the "re-try" behaviour? i.e. when you delete a profile and the one you that would be loaded gets deleted before it has a chance to load (so that the code re-tries).
https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:400: continue; This is a rebase gone wrong. Same for the two above. :) On 2013/06/12 21:02:31, Alexei Svitkine wrote: > Nit: Indention looks wrong and inner loop shouldn't have {}'s if its a single > line. > > By the way, did you actually mean to remove these LOG statements? https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1075: // On the Mac, the browser process is not killed when all browser windows We could, but it would be a wasted operation -- on the other platforms, deleting the active profile (with no other profiles opened) just kills the browser process, so there's not much point waiting around to load a profile that won't be used. Do you think we should do it anyway? On 2013/06/12 21:16:49, sail wrote: > Could we just implement this on all platforms?
https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1075: // On the Mac, the browser process is not killed when all browser windows On 2013/06/13 14:23:55, Monica Dinculescu wrote: > We could, but it would be a wasted operation -- on the other platforms, deleting > the active profile (with no other profiles opened) just kills the browser > process, so there's not much point waiting around to load a profile that won't > be used. Do you think we should do it anyway? Up to you. There might be weird edge cases. For example, if you have a background extension running and the browser process is not killed. > > On 2013/06/12 21:16:49, sail wrote: > > Could we just implement this on all platforms? >
https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1075: // On the Mac, the browser process is not killed when all browser windows On 2013/06/13 14:33:50, sail wrote: > On 2013/06/13 14:23:55, Monica Dinculescu wrote: > > We could, but it would be a wasted operation -- on the other platforms, > deleting > > the active profile (with no other profiles opened) just kills the browser > > process, so there's not much point waiting around to load a profile that won't > > be used. Do you think we should do it anyway? > > Up to you. > > There might be weird edge cases. For example, if you have a background extension > running and the browser process is not killed. Wouldn't any extension that's running still be running under a profile? So presumably if its profile is deleted, it should also be stopped. (Or if it isn't, that would be a separate bug.) > > > > > On 2013/06/12 21:16:49, sail wrote: > > > Could we just implement this on all platforms? > > >
https://codereview.chromium.org/14923004/diff/57001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/14923004/diff/57001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:223: [app_controller_ profileWasRemoved:profile_name]; On 2013/06/12 21:16:49, sail wrote: > It would be more direct to use profile_path to reference the deleted profile. Done. https://codereview.chromium.org/14923004/diff/57001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:782: if (profileName == lastProfileName) { On 2013/06/12 21:02:31, Alexei Svitkine wrote: > Nit: No {}'s Done. https://codereview.chromium.org/14923004/diff/57001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1174: Profile* lastUsedProfile = On 2013/06/12 21:02:31, Alexei Svitkine wrote: > Nit: Just return it directly. Done. https://codereview.chromium.org/14923004/diff/57001/chrome/browser/app_contro... File chrome/browser/app_controller_mac_unittest.mm (right): https://codereview.chromium.org/14923004/diff/57001/chrome/browser/app_contro... chrome/browser/app_controller_mac_unittest.mm:94: ProfileRemovalObserver observer(manager->profile_manager(), On 2013/06/12 21:16:49, sail wrote: > do you need this observer? Could you just do a check below the > ScheduleProfileForDeletion() call? Done. https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1126: BrowserList::CloseAllBrowsersWithProfile(profile); On 2013/06/12 21:02:31, Alexei Svitkine wrote: > Nit:Indention is wrong. Done. https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_manager_unittest.cc (right): https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager_unittest.cc:502: ProfileInfoCache& cache = profile_manager->GetProfileInfoCache(); On 2013/06/12 21:42:23, Alexei Svitkine wrote: > Either make it const or a pointer. > > Style guide disallows non-const references. (And yes, I know ProfileInfoCache is > used like that from many places.) Done. https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager_unittest.cc:517: string16(), string16(), false); On 2013/06/12 21:42:23, Alexei Svitkine wrote: > Can you refactor this block into a helper function that just takes dest_path1 > and mock_observer, so the test is more readable? Done. https://codereview.chromium.org/14923004/diff/57001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager_unittest.cc:534: base::Bind(&CheckLastUsedProfile, dest_path2, "New Profile 2")); On 2013/06/12 21:42:23, Alexei Svitkine wrote: > Do you need this class? > > Can't you just do the calls below (schedule deletion, run until idle) and then > assert that the profile was deleted and then check last used profile? Done.
lgtm
In addition to what we discussed offline, here's some nits. https://codereview.chromium.org/14923004/diff/82001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/14923004/diff/82001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1093: // For OS_MACOSX the pref is updated in the callback Nit: comment wrapping is weird https://codereview.chromium.org/14923004/diff/82001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_manager_unittest.cc (right): https://codereview.chromium.org/14923004/diff/82001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager_unittest.cc:485: "New Profile 2"); Same comments here as for the test below. https://codereview.chromium.org/14923004/diff/82001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager_unittest.cc:528: g_browser_process->profile_manager()->GetLastUsedProfile()->GetPath(), Nit: you already have a local pointer to profile_manager, no need to ask g_browser_process for it. https://codereview.chromium.org/14923004/diff/82001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager_unittest.cc:531: g_browser_process->local_state()->GetString(prefs::kProfileLastUsed), Ditto for local_state https://codereview.chromium.org/14923004/diff/82001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager_unittest.cc:532: "New Profile 2"); Usually, we put the expected value as the first param to the macros. Please fix here and above.
https://codereview.chromium.org/14923004/diff/82001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/14923004/diff/82001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1093: // For OS_MACOSX the pref is updated in the callback On 2013/06/14 22:04:58, Alexei Svitkine wrote: > Nit: comment wrapping is weird Done. https://codereview.chromium.org/14923004/diff/82001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_manager_unittest.cc (right): https://codereview.chromium.org/14923004/diff/82001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager_unittest.cc:528: g_browser_process->profile_manager()->GetLastUsedProfile()->GetPath(), On 2013/06/14 22:04:58, Alexei Svitkine wrote: > Nit: you already have a local pointer to profile_manager, no need to ask > g_browser_process for it. Done. https://codereview.chromium.org/14923004/diff/82001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager_unittest.cc:531: g_browser_process->local_state()->GetString(prefs::kProfileLastUsed), On 2013/06/14 22:04:58, Alexei Svitkine wrote: > Ditto for local_state Done. https://codereview.chromium.org/14923004/diff/82001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager_unittest.cc:532: "New Profile 2"); On 2013/06/14 22:04:58, Alexei Svitkine wrote: > Usually, we put the expected value as the first param to the macros. Please fix > here and above. Done.
https://codereview.chromium.org/14923004/diff/90001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/14923004/diff/90001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1058: ProfilesToDelete().end(), cur_path) != ProfilesToDelete().end(); Can you make a helper function for this, since it's used in a few places? https://codereview.chromium.org/14923004/diff/90001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1110: const base::FilePath& last_non_managed_profile_path, Now that you have this, can you remove the redundant |last_non_managed_profile| param? https://codereview.chromium.org/14923004/diff/90001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1114: Nit: No empty line here. https://codereview.chromium.org/14923004/diff/90001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1117: if (status != Profile::CREATE_STATUS_CREATED) { Please check specific statuses rather than not a specific status. This way, if another status is added, it won't start triggering this code unintentionally.
https://codereview.chromium.org/14923004/diff/90001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/14923004/diff/90001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1058: ProfilesToDelete().end(), cur_path) != ProfilesToDelete().end(); On 2013/06/18 21:45:57, Alexei Svitkine wrote: > Can you make a helper function for this, since it's used in a few places? Done. https://codereview.chromium.org/14923004/diff/90001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1110: const base::FilePath& last_non_managed_profile_path, On 2013/06/18 21:45:57, Alexei Svitkine wrote: > Now that you have this, can you remove the redundant |last_non_managed_profile| > param? Done. https://codereview.chromium.org/14923004/diff/90001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1114: On 2013/06/18 21:45:57, Alexei Svitkine wrote: > Nit: No empty line here. Done. https://codereview.chromium.org/14923004/diff/90001/chrome/browser/profiles/p... chrome/browser/profiles/profile_manager.cc:1117: if (status != Profile::CREATE_STATUS_CREATED) { On 2013/06/18 21:45:57, Alexei Svitkine wrote: > Please check specific statuses rather than not a specific status. This way, if > another status is added, it won't start triggering this code unintentionally. Done.
You also have a compile error on win. https://codereview.chromium.org/14923004/diff/102001/chrome/browser/profiles/... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/14923004/diff/102001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager.cc:1112: status == Profile::CREATE_STATUS_LOCAL_FAIL || Should we actually be doing the recursion in the fail status case? From what I remember, we couldn't get it to happen in the tests, but it seems wrong to set prefs::kProfileLastUsed to last_non_managed_profile_path if it failed to load. https://codereview.chromium.org/14923004/diff/102001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager.cc:1116: // deleted, then retry deleting this profile to redo the logic and load nit: "and load" -> "to load" https://codereview.chromium.org/14923004/diff/102001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager.cc:1150: bool ProfileManager::IsProfileMarkedForDeletion( Move this to the anon namespace at the top of the file instead. https://codereview.chromium.org/14923004/diff/102001/chrome/browser/profiles/... File chrome/browser/profiles/profile_manager_unittest.cc (right): https://codereview.chromium.org/14923004/diff/102001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager_unittest.cc:123: virtual void CreateProfileAsync(ProfileManager* manager, Why is this virtual? Also, it needs a comment. https://codereview.chromium.org/14923004/diff/102001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager_unittest.cc:524: // ProfileManager needs to recursively select a different next profile. How does this work on platforms other than Mac? The recursion doesn't happen there, so is the test still valid in that case? Same question for the other tests. https://codereview.chromium.org/14923004/diff/102001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager_unittest.cc:526: Nit: Remove empty line.
https://codereview.chromium.org/14923004/diff/102001/chrome/browser/profiles/... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/14923004/diff/102001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager.cc:1112: status == Profile::CREATE_STATUS_LOCAL_FAIL || I've added a DCHECK. On 2013/06/19 14:58:31, Alexei Svitkine wrote: > Should we actually be doing the recursion in the fail status case? > > From what I remember, we couldn't get it to happen in the tests, but it seems > wrong to set prefs::kProfileLastUsed to last_non_managed_profile_path if it > failed to load. https://codereview.chromium.org/14923004/diff/102001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager.cc:1116: // deleted, then retry deleting this profile to redo the logic and load On 2013/06/19 14:58:31, Alexei Svitkine wrote: > nit: "and load" -> "to load" Done. https://codereview.chromium.org/14923004/diff/102001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager.cc:1150: bool ProfileManager::IsProfileMarkedForDeletion( On 2013/06/19 14:58:31, Alexei Svitkine wrote: > Move this to the anon namespace at the top of the file instead. Done. https://codereview.chromium.org/14923004/diff/102001/chrome/browser/profiles/... File chrome/browser/profiles/profile_manager_unittest.cc (right): https://codereview.chromium.org/14923004/diff/102001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager_unittest.cc:123: virtual void CreateProfileAsync(ProfileManager* manager, On 2013/06/19 14:58:31, Alexei Svitkine wrote: > Why is this virtual? Also, it needs a comment. Done. https://codereview.chromium.org/14923004/diff/102001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager_unittest.cc:524: // ProfileManager needs to recursively select a different next profile. Tests shouldn't work, so I've ifdef'ed them. On 2013/06/19 14:58:31, Alexei Svitkine wrote: > How does this work on platforms other than Mac? The recursion doesn't happen > there, so is the test still valid in that case? Same question for the other > tests. https://codereview.chromium.org/14923004/diff/102001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager_unittest.cc:526: On 2013/06/19 14:58:31, Alexei Svitkine wrote: > Nit: Remove empty line. Done.
LGTM with one last round of nits below: https://codereview.chromium.org/14923004/diff/110001/chrome/browser/app_contr... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/14923004/diff/110001/chrome/browser/app_contr... chrome/browser/app_controller_mac.mm:222: // so that it can correctly update its pointer to the last used profile. Nit: "Notify the AppController so it can correctly update its pointer to the last used profile." https://codereview.chromium.org/14923004/diff/110001/chrome/browser/app_contr... chrome/browser/app_controller_mac.mm:1171: return g_browser_process->profile_manager()->GetLastUsedProfile(); Nit: It doesn't seem like you're changing any logic in this function, so can you remove the diff to avoid the extra code churn? https://codereview.chromium.org/14923004/diff/110001/chrome/browser/app_contr... File chrome/browser/app_controller_mac_unittest.mm (right): https://codereview.chromium.org/14923004/diff/110001/chrome/browser/app_contr... chrome/browser/app_controller_mac_unittest.mm:36: extension_event_router_forwarder_; What's this class needed for? If it's not needed, please remove it along with the include. https://codereview.chromium.org/14923004/diff/110001/chrome/browser/profiles/... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/14923004/diff/110001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager.cc:165: bool IsProfileMarkedForDeletion(const base::FilePath& profile_dir) { Nit: Rename param to |profile_path|. https://codereview.chromium.org/14923004/diff/110001/chrome/browser/profiles/... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/14923004/diff/110001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager.h:335: // the kProfileLastUsed preference to |last_non_managed_profile|. Nit: last_non_managed_profile_path https://codereview.chromium.org/14923004/diff/110001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager.h:337: const base::FilePath& profile_to_delete_dir, Nit: Rename to |profile_to_delete_path| so that it's named consistently with the variable below. https://codereview.chromium.org/14923004/diff/110001/chrome/browser/profiles/... File chrome/browser/profiles/profile_manager_unittest.cc (right): https://codereview.chromium.org/14923004/diff/110001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager_unittest.cc:131: UTF8ToUTF16(name), Nice, I see you're now always setting the profile avatar name! Maybe this will fix the flakyness of the other tests. After this CL lands, it may be worthwhile to try to re-enable the other tests (in a separate CL), to see if they're no longer flaky.
+ nico for owners stamp on: chrome/browser/app_controller_mac.h chrome/browser/app_controller_mac.mm chrome/browser/app_controller_mac_unittest.mm
(Don't forget to address my nits above too.) On Tue, Jun 25, 2013 at 3:05 PM, <noms@chromium.org> wrote: > + nico for owners stamp on: > chrome/browser/app_controller_**mac.h > chrome/browser/app_controller_**mac.mm <http://app_controller_mac.mm> > chrome/browser/app_controller_**mac_unittest.mm<http://app_controller_mac_unittest.mm> > > https://codereview.chromium.**org/14923004/<https://codereview.chromium.org/1... >
lgtm re CL description: `git log` doesn't wrap CL descriptions so try to keep lines shorter than 80 cols or so. A good format is imho """Short one line description of the change. (blank line) Some more elaboration explaining _why_ this change is done. possibly a BUG= line possibly a TEST= line with manual test steps. """ (You description is fine except for the formatting.) https://codereview.chromium.org/14923004/diff/110001/chrome/browser/app_contr... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/14923004/diff/110001/chrome/browser/app_contr... chrome/browser/app_controller_mac.mm:201: : profile_manager_(profile_manager), nit: ':' is indented 4 relative to the constructor name, not relative to the parameter list
https://codereview.chromium.org/14923004/diff/110001/chrome/browser/app_contr... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/14923004/diff/110001/chrome/browser/app_contr... chrome/browser/app_controller_mac.mm:201: : profile_manager_(profile_manager), On 2013/06/25 19:20:20, Nico wrote: > nit: ':' is indented 4 relative to the constructor name, not relative to the > parameter list Done. https://codereview.chromium.org/14923004/diff/110001/chrome/browser/app_contr... chrome/browser/app_controller_mac.mm:222: // so that it can correctly update its pointer to the last used profile. On 2013/06/22 14:14:09, Alexei Svitkine wrote: > Nit: "Notify the AppController so it can correctly update its pointer to the > last used profile." Done. https://codereview.chromium.org/14923004/diff/110001/chrome/browser/app_contr... chrome/browser/app_controller_mac.mm:1171: return g_browser_process->profile_manager()->GetLastUsedProfile(); On 2013/06/22 14:14:09, Alexei Svitkine wrote: > Nit: It doesn't seem like you're changing any logic in this function, so can you > remove the diff to avoid the extra code churn? Done. https://codereview.chromium.org/14923004/diff/110001/chrome/browser/app_contr... File chrome/browser/app_controller_mac_unittest.mm (right): https://codereview.chromium.org/14923004/diff/110001/chrome/browser/app_contr... chrome/browser/app_controller_mac_unittest.mm:36: extension_event_router_forwarder_; Blurg, it's leftover dead code. Done. On 2013/06/22 14:14:09, Alexei Svitkine wrote: > What's this class needed for? If it's not needed, please remove it along with > the include. https://codereview.chromium.org/14923004/diff/110001/chrome/browser/profiles/... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/14923004/diff/110001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager.cc:165: bool IsProfileMarkedForDeletion(const base::FilePath& profile_dir) { On 2013/06/22 14:14:09, Alexei Svitkine wrote: > Nit: Rename param to |profile_path|. Done. https://codereview.chromium.org/14923004/diff/110001/chrome/browser/profiles/... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/14923004/diff/110001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager.h:335: // the kProfileLastUsed preference to |last_non_managed_profile|. On 2013/06/22 14:14:09, Alexei Svitkine wrote: > Nit: last_non_managed_profile_path Done. https://codereview.chromium.org/14923004/diff/110001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager.h:337: const base::FilePath& profile_to_delete_dir, On 2013/06/22 14:14:09, Alexei Svitkine wrote: > Nit: Rename to |profile_to_delete_path| so that it's named consistently with the > variable below. Done. https://codereview.chromium.org/14923004/diff/110001/chrome/browser/profiles/... File chrome/browser/profiles/profile_manager_unittest.cc (right): https://codereview.chromium.org/14923004/diff/110001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager_unittest.cc:131: UTF8ToUTF16(name), Yes and no. We would still have to set the avatar url, I think, as the username is ignored if that isn't set, but I didn't want to experiment with that on this CL. It's on my to-do list. :) On 2013/06/22 14:14:09, Alexei Svitkine wrote: > Nice, I see you're now always setting the profile avatar name! > > Maybe this will fix the flakyness of the other tests. After this CL lands, it > may be worthwhile to try to re-enable the other tests (in a separate CL), to see > if they're no longer flaky.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noms@chromium.org/14923004/129002
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noms@chromium.org/14923004/129002
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noms@chromium.org/14923004/156001
Retried try job too often on linux_chromeos for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noms@chromium.org/14923004/180001
Message was sent while issue was closed.
Change committed as 209035 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
