Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1051843002:
Use last active profile instead of default profile in profile manager (Closed)
|
Created:
5 years, 8 months ago by kaliamoorthi Modified:
5 years, 8 months ago CC:
chromium-reviews, rginda+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse last active profile instead of default profile in profile manager
When profiles are deleted, new profiles are created in the directory
Profile X. These directories are stored in last active profile. In metro
mode, on initialization rather than last active profile, default profile
corresponding to the directory Default is used resulting in problems
on profile deletion. This CL makes ProfileManager::GetActiveUserProfile
return last active profile instead of default profile for windows build
to solve the problem.
BUG=450192
Committed: https://crrev.com/e4bc3e8aae7ab612458ba94b0c6065dab7ae1da9
Cr-Commit-Position: refs/heads/master@{#325440}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Moves the logic to ProfileManager::GetActiveUserProfile for windows #Messages
Total messages: 19 (4 generated)
kaliamoorthi@chromium.org changed reviewers: + skuhne@chromium.org
PTAL
skuhne@chromium.org changed reviewers: + dpolukhin@chromium.org
+dpolukhin since he is probably a better reviewer for this change.
Yeah... I'm not sure that I'm better reviewer. This CL changes desktop behavior that I'm not familiar. In general it looks like proposed change change behavior of GetActiveUserOrOffTheRecordProfileFromPath to return last used profile instead of fixed one. I think it requires reviewing all call paths to make sure that if doesn't break assumptions. Perhaps safer change is to change caller to ask last used profile directly if it really required in that context. Last used profile on desktop is not 100% equivalent of ActiveUser on Chrome OS.
On 2015/04/02 11:10:17, Dmitry Polukhin wrote: > Yeah... I'm not sure that I'm better reviewer. This CL changes desktop behavior > that I'm not familiar. In general it looks like proposed change change behavior > of GetActiveUserOrOffTheRecordProfileFromPath to return last used profile > instead of fixed one. I think it requires reviewing all call paths to make sure > that if doesn't break assumptions. Perhaps safer change is to change caller to > ask last used profile directly if it really required in that context. Last used > profile on desktop is not 100% equivalent of ActiveUser on Chrome OS. Thanks for your input, I'll wait for skuhne@ to return to review this CL. Cheers, Prabhu.
Sorry was OOO for a week and just came back. I checked how we got there and under NON ChromeOS there is only a single call of interest: GetActiveUserProfile (GetPrimaryUserProfile does not have a special meaning on non ChromeOS devices). GetActiveUserProfile is called from 75 different locations. Very hard to say if there is a special condition which might get broken (e.g. OOB, first user, ..). As such I tend to agree with dpolukhin's assessment. Unintended side effects might occur. Is there any way to isolate the problem more towards a special use case of the function (what the bug describes) and change it there instead?
On 2015/04/13 19:12:10, Mr4D wrote: > Sorry was OOO for a week and just came back. > > I checked how we got there and under NON ChromeOS there is only a single > call of interest: GetActiveUserProfile (GetPrimaryUserProfile does not > have a special meaning on non ChromeOS devices). > > GetActiveUserProfile is called from 75 different locations. Very hard to > say if there is a special condition which might get broken (e.g. OOB, > first user, ..). > > As such I tend to agree with dpolukhin's assessment. Unintended side > effects might occur. > > Is there any way to isolate the problem more towards a special use case > of the function (what the bug describes) and change it there instead? From my discussion with Dmitry, I learned that GetActiveUserProfile is not clearly defined in windows metro mode. But according to the current code, GetActiveUserProfile always point to Default. This already seems wrong to me, since GetActiveUserProfile wont be Default after a profile deletion.
On 2015/04/14 11:27:31, kaliamoorthi wrote: > On 2015/04/13 19:12:10, Mr4D wrote: > > Sorry was OOO for a week and just came back. > > > > I checked how we got there and under NON ChromeOS there is only a single > > call of interest: GetActiveUserProfile (GetPrimaryUserProfile does not > > have a special meaning on non ChromeOS devices). > > > > GetActiveUserProfile is called from 75 different locations. Very hard to > > say if there is a special condition which might get broken (e.g. OOB, > > first user, ..). > > > > As such I tend to agree with dpolukhin's assessment. Unintended side > > effects might occur. > > > > Is there any way to isolate the problem more towards a special use case > > of the function (what the bug describes) and change it there instead? > > From my discussion with Dmitry, I learned that GetActiveUserProfile is not > clearly defined in windows metro mode. But according to the current code, > GetActiveUserProfile always point to Default. This already seems wrong to me, > since GetActiveUserProfile wont be Default after a profile deletion. I mean active profile wont be Default after profile deletion.
I have just noticed that I didn't send my comments earlier. So I do this now. https://codereview.chromium.org/1051843002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.cc (left): https://codereview.chromium.org/1051843002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.cc:1110: default_profile_dir = default_profile_dir.Append(GetInitialProfileDir()); Looking into line 473 above there is a TODO comment from mirandac which makes me wonder what that is about, but then again - that was from 2011 and it is questionable on how this function is/was used since then.
On 2015/04/15 19:26:21, Mr4D wrote: > I have just noticed that I didn't send my comments earlier. So I do this now. > > https://codereview.chromium.org/1051843002/diff/1/chrome/browser/profiles/pro... > File chrome/browser/profiles/profile_manager.cc (left): > > https://codereview.chromium.org/1051843002/diff/1/chrome/browser/profiles/pro... > chrome/browser/profiles/profile_manager.cc:1110: default_profile_dir = > default_profile_dir.Append(GetInitialProfileDir()); > Looking into line 473 above there is a TODO comment from mirandac which makes me > wonder what that is about, but then again - that was from 2011 and it is > questionable on how this function is/was used since then. I moved the logic to ProfileManager::GetActiveUserProfile as we discussed. Please note that with this change there is absolutely no impact for chrome os. If there is a macro for metro mode I can also restrict the change just to metro mode.
Patchset #2 (id:20001) has been deleted
lgtm
On 2015/04/16 14:12:08, Mr4D wrote: > lgtm Thanks for the review.
The CQ bit was checked by kaliamoorthi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1051843002/40001
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e4bc3e8aae7ab612458ba94b0c6065dab7ae1da9 Cr-Commit-Position: refs/heads/master@{#325440}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:40001) has been created in https://codereview.chromium.org/1113333005/ by kaliamoorthi@chromium.org. The reason for reverting is: This CL breaks guest mode in Windows 8 metro mode crbug.com/480400. Reverting it to find a suitable fix that solves both 450192 and 480400.. |
