Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2047483003:
Add fallback behavior if the last used profile cannot initialize (Closed)
DescriptionAdd fallback behavior if the last used profile cannot initialize
Before this CL, if the last used profile cannot be initialized, it will
be a silent exit in Windows and a CHECK failure in other platforms.
In this CL a list of possible fall-back behavior are used (from the top
to the bottom):
(1) open other last opened profiles which was not signed out.
(2) open the user manager (guest profile & system profile)
(3) open other profiles which was not signed out.
(4) at last, show a message box and do not start chromium.
BUG=614753
Committed: https://crrev.com/2873496dd5bb12be052aa1c5a341637faade926d
Cr-Commit-Position: refs/heads/master@{#409023}
Patch Set 1 : Updated code #
Total comments: 2
Patch Set 2 : Added translatable strings and histogram to the dialog boxes #
Total comments: 7
Patch Set 3 : Rebase #
Total comments: 1
Patch Set 4 : Refactored startup profile choosing code, reduced number of messages, bug fix (oops) #
Total comments: 36
Patch Set 5 : Add tests #Patch Set 6 : Fix startup failure if unable to create system profile. #Patch Set 7 : Respond to pkasting@'s comments, minor changes to test code #Patch Set 8 : Added a missing test (oops) #
Total comments: 2
Patch Set 9 : Handle the case where the selected profile is not in ProfileAttributesStorage #
Total comments: 56
Patch Set 10 : Respond to pkasting@'s comments #
Total comments: 8
Patch Set 11 : Copy base/test/* from CL 2061593002 #Patch Set 12 : Respond to pkasting@'s comments #Patch Set 13 : Respond to comments, copy base/test/* from CL 2061593002 again. #
Total comments: 66
Patch Set 14 : Respond to pkasting@'s comments #Patch Set 15 : Fix clang compile error #
Total comments: 16
Patch Set 16 : Rebase #Patch Set 17 : Add missing #include (left out by bad rebase :-( ) #Patch Set 18 : Respond to comments. #Patch Set 19 : Fix a mistake in a comment. (Didn't check Atom reflow output, sorry :-( ) #Messages
Total messages: 89 (43 generated)
Description was changed from ========== Add fallback behavior if the last used profile cannot initialize BUG=614753 ========== to ========== Add fallback behavior if the last used profile cannot initialize Before this CL, if the last used profile cannot be initialized, it will be a silent exit in Windows and a CHECK failure in other platforms. In this CL a list of possible fall-back behavior are used (from the top to the bottom): (1) open other last opened profiles which does not require signin. (2) open the user manager (guest profile) (3) open other profiles which does not require signin. (4) at last, show a message box and do not start chromium. BUG=614753 ==========
lwchkg@chromium.org changed reviewers: + anthonyvd@chromium.org, pkasting@chromium.org
Description was changed from ========== Add fallback behavior if the last used profile cannot initialize Before this CL, if the last used profile cannot be initialized, it will be a silent exit in Windows and a CHECK failure in other platforms. In this CL a list of possible fall-back behavior are used (from the top to the bottom): (1) open other last opened profiles which does not require signin. (2) open the user manager (guest profile) (3) open other profiles which does not require signin. (4) at last, show a message box and do not start chromium. BUG=614753 ========== to ========== Add fallback behavior if the last used profile cannot initialize Before this CL, if the last used profile cannot be initialized, it will be a silent exit in Windows and a CHECK failure in other platforms. In this CL a list of possible fall-back behavior are used (from the top to the bottom): (1) open other last opened profiles which does not require signin. (2) open the user manager (guest profile) (3) open other profiles which does not require signin. (4) at last, show a message box and do not start chromium. BUG=614753 ==========
The CQ bit was checked by lwchkg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2047483003/1
Here's the second CL of bug 614753, which takes care of the last used profile. There should be one more CL about ProfileViewChooser, which is supposed to fix crashes by failures to open profiles. Peter: PTAL. Anthony: FYI. https://codereview.chromium.org/2047483003/diff/1/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2047483003/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:415: base::UTF8ToUTF16("Cannot open or create the specified profile.")); We need to get help from the UI team to determine the exact message in this message box. Who should I contact? Anyway, I know that it is a bad idea to hard-code strings. I just want a confirmation that a message box to be shown here before working on translatable strings. https://codereview.chromium.org/2047483003/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:467: "The user data directory is possibly corrupted.")); Same for here. Anyway, should we also add some UMA histogram to log this error? After the patch the crash is not there anymore, so we need some other kind of logging. If yes, then who is going to own the histogram(s)? (I'm not a Googler now, so it sholudn't be me.)
Patchset #1 (id:1) has been deleted
lwchkg@chromium.org changed reviewers: + sky@chromium.org
+sky Messed up with a old commit and wrong list of reviewers (sorry to Peter). So here's a resend: Here's the second CL of bug 614753, which takes care of the last used profile. (Scott: the first CL, listed as dependency, takes care of last opened profiles, which is the immediate reason of the crash.) Scott: PTAL Peter, Anthony: FYI. Please note that another CL about ProfileChooserView is needed to fully fix the bug. https://codereview.chromium.org/2047483003/diff/20001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2047483003/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:434: return nullptr; We need to get help from the UI team to determine the exact message in this message box. Who should I contact? Anyway, I know that I should not hard-code strings. Just want a confirmation that a message box is the right UI be shown here before working on translatable strings. https://codereview.chromium.org/2047483003/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:487: "The user data directory is possibly corrupted.")); Same for here. Anyway, should we also add some UMA histogram to log this error? After the patch the crash is not there anymore, so we need some other kind of logging. If yes, then who is going to own the histogram(s)? (I'm not a Googler now, so it sholudn't be me.)
sky@chromium.org changed reviewers: + brettw@chromium.org - sky@chromium.org
sky->brettw as Brett is reviewing your dependent patch and is an owner of this as well.
The CQ bit was checked by lwchkg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2047483003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Brett: ping. Anyway feel free to defer the review to another owner because what sky@ said is unfortunately not true. (Actually only the test of the patch, which was finally not landed in that CL, was being reviewed by Brett.)
falling back to other profiles seems like a potentially dangerous thing me. Is there a design doc that covers the higher-level goal of this?
On 2016/06/13 20:21:34, brettw wrote: > falling back to other profiles seems like a potentially dangerous thing me. Is > there a design doc that covers the higher-level goal of this? No. All I have is some prior discussion with Peter Kasting in a previous CL (starting in https://codereview.chromium.org/2021253002/#msg22). Here's a copy below: (Note: the unquoted words was by me.) On 2016/06/01 17:49:19, Peter Kasting wrote: > On 2016/06/01 17:41:32, WC Leung wrote: > > Anyway, we need some better fallback behavior for case (2), possible one or > more > > of the following: > > (a) crash nicely. (i.e. with CHECK(false) or LOG(error), plus a descriptive > > message.) > > Don't do this. Acknowledged. > > > (b) try to open other profiles. (If |last_opened_profiles| is not empty, I > think > > we're going to do this.) > > Yes, I would do this if there are any other previous profiles to open. BTW, should we open other profiles if there is no |last_opened_profiles| to open? > > > (c) use a guest profile. (If we use this one, should we output a message in > > Chrome?) > > If there are no other previous profiles to open, either use guest mode or create > a new profile (what if either/both of these fail?), and tell the user we > couldn't open (names of profiles). I'd maybe add a Learn More link to a help > center article about what to do here, and show this message in both cases above. Guest mode: sounds good to me. Hopefully this always succeed. If fail, then this is a bug (and unfortunately needs to crash). New profile: won't bother trying because creating a profile is more likely to fail than succeed. When we reach this step, we DID try to create other profiles, but failed anyway. Show message: sounds good to me. Help center article: I'm not familiar with this. Anthony: WDYT? ---- And now here's my answer to the "potentially dangerous" thing. I hope that I answer correctly in a security perspective, but can't be sure because I'm not an expert: 1. Crash: this is the baseline for comparison. The current "buggy" state is crashing anyway. Anyway, if we have chosen not to start Chromium, we should display a nicely formatted dialog box to inform users. We also want to have UMA histograms. 2. Profile that are signed out: Do not do this. These profile needs to be signed in properly before being opened. Otherwise this is likely a security problem. Note that in all cases below, the profile that need sign-in should be filtered out. 3. Other last opened profiles: safe. The behavior before this CL is to open all of these profiles. So we are preserving existing behavior. 4. Guest profile (i.e. user manager): safe. Reference to existing behavior: Somehow if the last opened profile is signed out (I still do not understand when it does happen), it reverts to the user manager. 5. Other non last opened profiles that does not require sign-in: This is open to discussion. To me this is okay because sign-in is not required. And we probably want to do this because we don't want to lock users out of Chromium. (Note: I'm referring to https://bugs.chromium.org/p/chromium/issues/detail?id=528038#c12 by finkm@ when I say "lock users out".) Anyway, WDYT?
I understand this patch more now. I thought initially it handled a more broad class of error which made me nervous about randomly picking other profiles. I do think we should have a test for this before landing. This code will basically never be run on any bot or by any developer, making the test for this obscure case much more important. I remember some problems with the previous test. It would be nice if we could express this as a test that, in the body of the test, sets the permissions so the directory can't be created, launches the browser on that directory, verifies the fallback behavior. https://codereview.chromium.org/2047483003/diff/40001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2047483003/diff/40001/chrome/app/generated_re... chrome/app/generated_resources.grd:11280: + <message name="IDS_COULDNT_OPEN_ANY_PROFILE_ERROR" desc="Error displayed on startup failure, when none of the existing profiles can be opened correctly."> I think I would not bother adding these 2 new messages and just re-use the IDS_COULDNT_OPEN_PROFILE_ERROR message for all of the related errors. The difference between these two cases to an end-user is not actionable and it just adds more messages we need to maintain. https://codereview.chromium.org/2047483003/diff/40001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2047483003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:437: // If the last used profile is unable to initialize, see if any of other last Can the code added from here be moved to a helper function "PickFallbackProfile" (or something) to avoid making this existing function so large? Potentially then you could factor it so that the error dialog code only happens once (you conditionally call the fallback when no profile is explicitly specified, then unconditionally fail at the end if profile is still null). https://codereview.chromium.org/2047483003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:443: // Get the first profile that is not signed out.. nit: remove extra "."
Patchset #4 (id:80001) has been deleted
New patch uploaded. Brett: The test were not added yet. But I guess you're interested in the rest of the changes. Peter: Now you own chrome/browser/ui/*. PTAL. Anyway, the proposed tests will also be added to chrome/browser/ui as well. On 2016/06/14 00:02:07, brettw wrote: > I do think we should have a test for this before landing. This code will > basically never be run on any bot or by any developer, making the test for this > obscure case much more important. Agree with Brett that we should have the tests ready before landing. There are a lot of corner cases to handle, so we'd better play safe. (And I'll hopefully add the tests tomorrow.) https://codereview.chromium.org/2047483003/diff/40001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2047483003/diff/40001/chrome/app/generated_re... chrome/app/generated_resources.grd:11280: + <message name="IDS_COULDNT_OPEN_ANY_PROFILE_ERROR" desc="Error displayed on startup failure, when none of the existing profiles can be opened correctly."> On 2016/06/14 00:02:07, brettw wrote: > I think I would not bother adding these 2 new messages and just re-use the > IDS_COULDNT_OPEN_PROFILE_ERROR message for all of the related errors. The > difference between these two cases to an end-user is not actionable and it just > adds more messages we need to maintain. I think we still need to add one string if two strings are too much. IDS_COULDNT_OPEN_PROFILE_ERROR does not carry the notion that Chromium cannot start (I am using it in 2061593002). Here we need a string that clearly state that Chromium cannot start. https://codereview.chromium.org/2047483003/diff/40001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2047483003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:437: // If the last used profile is unable to initialize, see if any of other last On 2016/06/14 00:02:07, brettw wrote: > Can the code added from here be moved to a helper function "PickFallbackProfile" > (or something) to avoid making this existing function so large? Potentially then > you could factor it so that the error dialog code only happens once (you > conditionally call the fallback when no profile is explicitly specified, then > unconditionally fail at the end if profile is still null). SGTM. Agree that the current function is too long, so I've refactored it a little bit (now some code is moved to startup_browser_creator.cc). Now the code should be much more readable! https://codereview.chromium.org/2047483003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:443: // Get the first profile that is not signed out.. On 2016/06/14 00:02:07, brettw wrote: > nit: remove extra "." Oops. Thanks for catching. Done. https://codereview.chromium.org/2047483003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:450: .GetProfileAttributesWithPath(profile->GetPath(), profile should be last_opened_profile instead. Oops! Thanks for the refactoring the bug was spotted easily with shorter functions. https://codereview.chromium.org/2047483003/diff/60001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2047483003/diff/60001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:50: #include "chrome/browser/component_updater/cld_component_installer.h" Didn't know why this line didn't go away in my rebase. Maybe I "fixed" the merge conflict wrongly.
https://codereview.chromium.org/2047483003/diff/100001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2047483003/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:11304: + <message name="IDS_COULDNT_OPEN_PROFILE_ERROR" desc="Error displayed on startup when the profile can not be opened correctly due to problems reading or writing files in it."> Nit: cannot https://codereview.chromium.org/2047483003/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:11310: +Cannot start Chrome because your could not be opened correctly. You're missing the word "profile". https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:432: CHECK(profile) << "Cannot get default profile."; This block should be moved up into the #if defined(OS_CHROMEOS)... block above, since that's the only codepath which could get here with |profile| null. After that's done, the conditional just above this can be removed along with the return statement below. https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:907: &entry); Nit: Shorter and IMO more readable: auto storage = profile_manager->GetProfileAttributesStorage(); ProfileAttributesEntry* entry; bool has_entry = storage.GetProfileAttributesWithPath(profile_path, &entry); https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:909: profile = profile_manager->GetProfile( Nit: Prefer to break after '=' instead https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:921: // |last_opened_profiles|). At this stage we assume creation of all new Nit: Remove parenthetical (comment should explain why, code explains what) https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:922: // profile (including guest profile) to fail. Nit: This sentence isn't grammatical, and doesn't motivate why we would assume this. Explain. https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:924: ProfileManager::GetLastOpenedProfiles(); Nit: I'd just inline this into the loop below. https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:931: &entry); Nit: Shorter and IMO more readable: auto storage = profile_manager->GetProfileAttributesStorage(); for (Profile* last_opened_profile : last_opened_profiles) { ProfileAttributesEntry* entry; bool has_entry = storage.GetProfileAttributesWithPath( last_opened_profile->GetPath(), &entry); This also lets you use |storage| below. https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:932: if (!has_entry || !entry->IsSigninRequired()) I'm confused about why we return here if !has_entry. https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:940: Nit: Unnecessary blank line https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:944: // If still fails, try to open any non-signin profile. Nit: any profile not requiring authentication? https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:947: .GetAllProfilesAttributes(); Nit: If you use |storage| here from my comment above, this can probably be inlined into the loop below. https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:185: // manager) if the above profile is signed out. Returns NULL if this profile Nit: null (2 places) What does "user manager" mean here? Even after reading the code in the .cc file I'm not sure how to best describe this. Be more detailed.
Description was changed from ========== Add fallback behavior if the last used profile cannot initialize Before this CL, if the last used profile cannot be initialized, it will be a silent exit in Windows and a CHECK failure in other platforms. In this CL a list of possible fall-back behavior are used (from the top to the bottom): (1) open other last opened profiles which does not require signin. (2) open the user manager (guest profile) (3) open other profiles which does not require signin. (4) at last, show a message box and do not start chromium. BUG=614753 ========== to ========== Add fallback behavior if the last used profile cannot initialize Before this CL, if the last used profile cannot be initialized, it will be a silent exit in Windows and a CHECK failure in other platforms. In this CL a list of possible fall-back behavior are used (from the top to the bottom): (1) open other last opened profiles which was not signed out. (2) open the user manager (guest profile & system profile) (3) open other profiles which was not signed out. (4) at last, show a message box and do not start chromium. BUG=614753 ==========
lwchkg@chromium.org changed reviewers: + mlerman@chromium.org
The CQ bit was checked by lwchkg@chromium.org
+Mike because he is a profile expert (hopefully back from O-O-O) Brett: Uploaded the tests. I wan't able to launch Chromium in the middle of a browser test. There is a base::LaunchProcess for launching processes, but somehow the behavior of launching with an existing process (i.e. the running browser test) present is different from launching without an existing process. e.g. If I launch "chrome --profile-directory=Test" with another Chromium process open, the profile is created async instead of sync (and crashes in different ways :-( ). This means the code in this CL is not executed. (Luckily the different crash is already handled in https://codereview.chromium.org/2061593002) So I continued to use PRE_ tests and SetUpUserDataDirectory. Didn't bother to remove the use of PRE_ tests with mocks, because SetUpUserDataDirectory was the most confusing part of the test instead of PRE_ tests. Instead I tried to make those three-part tests really readable. Please comment! Peter: Most of your comments are addressed. The tests are added to your directory as well. PTAL. Anthony and Mike: I've asked both of you two questions in the comments, one about the use of guest profile to refer to the user manager, and the other about the context of https://codereview.chromium.org/1021613002 (a CL by noms). https://codereview.chromium.org/2047483003/diff/100001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2047483003/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:11304: + <message name="IDS_COULDNT_OPEN_PROFILE_ERROR" desc="Error displayed on startup when the profile can not be opened correctly due to problems reading or writing files in it."> On 2016/06/16 06:37:49, Peter Kasting wrote: > Nit: cannot Done. https://codereview.chromium.org/2047483003/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:11310: +Cannot start Chrome because your could not be opened correctly. On 2016/06/16 06:37:49, Peter Kasting wrote: > You're missing the word "profile". Thanks and done. https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:432: CHECK(profile) << "Cannot get default profile."; On 2016/06/16 06:37:49, Peter Kasting wrote: > This block should be moved up into the #if defined(OS_CHROMEOS)... block above, > since that's the only codepath which could get here with |profile| null. > > After that's done, the conditional just above this can be removed along with the > return statement below. Done. https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:907: &entry); On 2016/06/16 06:37:49, Peter Kasting wrote: > Nit: Shorter and IMO more readable: > > auto storage = profile_manager->GetProfileAttributesStorage(); > ProfileAttributesEntry* entry; > bool has_entry = storage.GetProfileAttributesWithPath(profile_path, &entry); Done. https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:909: profile = profile_manager->GetProfile( On 2016/06/16 06:37:49, Peter Kasting wrote: > Nit: Prefer to break after '=' instead Done. https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:921: // |last_opened_profiles|). At this stage we assume creation of all new On 2016/06/16 06:37:49, Peter Kasting wrote: > Nit: Remove parenthetical (comment should explain why, code explains what) Done. https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:922: // profile (including guest profile) to fail. On 2016/06/16 06:37:49, Peter Kasting wrote: > Nit: This sentence isn't grammatical, and doesn't motivate why we would assume > this. Explain. Done. https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:924: ProfileManager::GetLastOpenedProfiles(); On 2016/06/16 06:37:49, Peter Kasting wrote: > Nit: I'd just inline this into the loop below. Done. https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:931: &entry); On 2016/06/16 06:37:49, Peter Kasting wrote: > Nit: Shorter and IMO more readable: > > auto storage = profile_manager->GetProfileAttributesStorage(); > for (Profile* last_opened_profile : last_opened_profiles) { > ProfileAttributesEntry* entry; > bool has_entry = storage.GetProfileAttributesWithPath( > last_opened_profile->GetPath(), &entry); > > This also lets you use |storage| below. Done. https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:932: if (!has_entry || !entry->IsSigninRequired()) On 2016/06/16 06:37:49, Peter Kasting wrote: > I'm confused about why we return here if !has_entry. AFAIK the only way for |has_entry| to be false is to run chrome with the switch "--profile-directory". BTW, just checked the blame, and it links to https://codereview.chromium.org/1021613002 and issue 421215 (sadly I don't have access to that issue). I have tried to start such a profile. Chromium started with the profile, but without sync because it is logged off. Here's a screenshot: https://drive.google.com/open?id=0BxDfZwJpDb1IRVlTWFowRWxpRnM Anthony and Mike: WDYT, and do you know why was it implemented this way? https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:940: On 2016/06/16 06:37:49, Peter Kasting wrote: > Nit: Unnecessary blank line Done. https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:942: return guest_profile; Anthony and Mike: do you know why we return the guest profile instead of the system profile when we intend to show the user manager? https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:944: // If still fails, try to open any non-signin profile. On 2016/06/16 06:37:49, Peter Kasting wrote: > Nit: any profile not requiring authentication? Thanks for reminder. I changed to "any profile that is not signed out". https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:947: .GetAllProfilesAttributes(); On 2016/06/16 06:37:49, Peter Kasting wrote: > Nit: If you use |storage| here from my comment above, this can probably be > inlined into the loop below. Done. https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:185: // manager) if the above profile is signed out. Returns NULL if this profile On 2016/06/16 06:37:49, Peter Kasting wrote: > Nit: null (2 places) Done. > > What does "user manager" mean here? Even after reading the code in the .cc file > I'm not sure how to best describe this. Be more detailed. The user manager (or UserManager) is an interface can usually be opened by (1) left click you name (or icon) in right side of the title bar. (2) select "Switch Person". The underlying url of the user manager is chrome://user-manager, which is supposed to be opened in the system profile. Not sure whether I should add a elaborate explanation in the source code, because it is going to be long. Note: There are a few different interfaces for switching profiles. The user manager is one of them. Another is called profile choose view, which is opened when you right click your name in the title bar. And yet another can be found in chrome://settings.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2047483003/180001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by lwchkg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2047483003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2047483003/diff/180001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2047483003/diff/180001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:897: // If we're using the --new-profile-management flag and this profile is signed it's not that the profile is signed out, it's that the profile is locked. Please update the comment.
https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:185: // manager) if the above profile is signed out. Returns NULL if this profile On 2016/06/19 17:56:19, WC Leung wrote: > On 2016/06/16 06:37:49, Peter Kasting wrote: > > Nit: null (2 places) > > Done. > > > > > What does "user manager" mean here? Even after reading the code in the .cc > file > > I'm not sure how to best describe this. Be more detailed. > > The user manager (or UserManager) is an interface can usually be opened by > (1) left click you name (or icon) in right side of the title bar. > (2) select "Switch Person". > The underlying url of the user manager is chrome://user-manager, which is > supposed to be opened in the system profile. > > Not sure whether I should add a elaborate explanation in the source code, > because it is going to be long. > > Note: There are a few different interfaces for switching profiles. The user > manager is one of them. Another is called profile choose view, which is opened > when you right click your name in the title bar. And yet another can be found in > chrome://settings. Sorry, I wasn't sufficiently clear. I know what the user manager is. What I'm confused by is the construction of a sentence which places "(user manager)" after "guest profile" as if the user manager is another name for the guest profile. It's like reading a sentence like "I went to the store and bought two cucumbers (football team)." "football team" is mystifying there. So I meant, clarify how the user manager relates to what's returned. This is related to the issue where it seems strange that we return the guest profile if we intend to show the user manager to begin with.
https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:932: if (!has_entry || !entry->IsSigninRequired()) On 2016/06/19 17:56:19, WC Leung wrote: > On 2016/06/16 06:37:49, Peter Kasting wrote: > > I'm confused about why we return here if !has_entry. > > AFAIK the only way for |has_entry| to be false is to run chrome with the switch > "--profile-directory". > > BTW, just checked the blame, and it links to > https://codereview.chromium.org/1021613002 and issue 421215 (sadly I don't have > access to that issue). > > I have tried to start such a profile. Chromium started with the profile, but > without sync because it is logged off. Here's a screenshot: > https://drive.google.com/open?id=0BxDfZwJpDb1IRVlTWFowRWxpRnM > > Anthony and Mike: WDYT, and do you know why was it implemented this way? ping for this comment. https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:944: // If still fails, try to open any non-signin profile. On 2016/06/19 17:56:19, WC Leung wrote: > On 2016/06/16 06:37:49, Peter Kasting wrote: > > Nit: any profile not requiring authentication? > > Thanks for reminder. I changed to "any profile that is not signed out". % mike's comment, I'll change all of these "signin" / "signed out" to "locked". https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:185: // manager) if the above profile is signed out. Returns NULL if this profile On 2016/06/20 02:25:50, Peter Kasting wrote: > Sorry, I wasn't sufficiently clear. I know what the user manager is. What I'm > confused by is the construction of a sentence which places "(user manager)" > after "guest profile" as if the user manager is another name for the guest > profile. It's like reading a sentence like "I went to the store and bought two > cucumbers (football team)." "football team" is mystifying there. > > So I meant, clarify how the user manager relates to what's returned. Acknowledged. > > This is related to the issue where it seems strange that we return the guest > profile if we intend to show the user manager to begin with. Looks like we should return a structure with a enum and optionally a profile, in which the enum class looks like OpenAtStartup {PROFILE, USER_MANAGER}. Returning a profile to request the user manager is confusing, and it blocks the possibility to open the real guest profile. (If the user manager fails to show, I believe that we should fallback to the real guest profile.) It's likely that we should start a new issue for this. (But well, this is already the third issue that we have come up with for one single issue.) Mike: Do you know what was happening with this code in the past? Why do we return the guest profile to denote the user manager? Was there any special reason in the design? https://codereview.chromium.org/2047483003/diff/180001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2047483003/diff/180001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:897: // If we're using the --new-profile-management flag and this profile is signed On 2016/06/20 01:09:36, Mike Lerman wrote: > it's not that the profile is signed out, it's that the profile is locked. Please > update the comment. Acknowledged.
https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:932: if (!has_entry || !entry->IsSigninRequired()) On 2016/06/20 15:44:24, WC Leung wrote: > On 2016/06/19 17:56:19, WC Leung wrote: > > On 2016/06/16 06:37:49, Peter Kasting wrote: > > > I'm confused about why we return here if !has_entry. > > > > AFAIK the only way for |has_entry| to be false is to run chrome with the > switch > > "--profile-directory". > > > > BTW, just checked the blame, and it links to > > https://codereview.chromium.org/1021613002 and issue 421215 (sadly I don't > have > > access to that issue). > > > > I have tried to start such a profile. Chromium started with the profile, but > > without sync because it is logged off. Here's a screenshot: > > https://drive.google.com/open?id=0BxDfZwJpDb1IRVlTWFowRWxpRnM > > > > Anthony and Mike: WDYT, and do you know why was it implemented this way? > > ping for this comment. In this case, couldn't the Guest Profile also be the reason we don't have an Entry? In which case, we'd want to go with last_opened_profile. We should always have a case for !has_entry. I think, though, that if (!has_entry) should attempt to use the GuestProfile.
I'm assuming you're still iterating with Mike and I should review after that, let me know if that's a mistake.
https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:932: if (!has_entry || !entry->IsSigninRequired()) On 2016/06/20 15:50:31, Mike Lerman wrote: > On 2016/06/20 15:44:24, WC Leung wrote: > > On 2016/06/19 17:56:19, WC Leung wrote: > > > On 2016/06/16 06:37:49, Peter Kasting wrote: > > > > I'm confused about why we return here if !has_entry. > > > > > > AFAIK the only way for |has_entry| to be false is to run chrome with the > > switch > > > "--profile-directory". > > > > > > BTW, just checked the blame, and it links to > > > https://codereview.chromium.org/1021613002 and issue 421215 (sadly I don't > > have > > > access to that issue). > > > > > > I have tried to start such a profile. Chromium started with the profile, but > > > without sync because it is logged off. Here's a screenshot: > > > https://drive.google.com/open?id=0BxDfZwJpDb1IRVlTWFowRWxpRnM > > > > > > Anthony and Mike: WDYT, and do you know why was it implemented this way? > > > > ping for this comment. > > In this case, couldn't the Guest Profile also be the reason we don't have an > Entry? In which case, we'd want to go with last_opened_profile. > Yes the guest profile can be a reason. BTW, I think returning the guest profile in the case is good enough. > We should always have a case for !has_entry. I think, though, that if > (!has_entry) should attempt to use the GuestProfile. This makes sense. Thanks. I am a bit concerned about this change may break existing users by forbidding them to access existing profiles. However, to have this happen the user need to copy the Chrome shortcut from Desktop into another folder (just renaming shortcuts is not enough), or deliberately execute chrome with a --profile-directory switch. Also, this code path is not crashing only for 1 year 3 months (after issue 421215 is fixed). I think it's still okay to make the proposed change. BTW, WDYT?
The CQ bit was checked by lwchkg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Mike: added the case for |has_entry| being false. PTAL. BTW, one file has been modified in c/b/profiles for this change. Peter, Brett: New patch uploaded. PTAL. https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:185: // manager) if the above profile is signed out. Returns NULL if this profile On 2016/06/20 15:44:24, WC Leung wrote: > On 2016/06/20 02:25:50, Peter Kasting wrote: > > Sorry, I wasn't sufficiently clear. I know what the user manager is. What > I'm > > confused by is the construction of a sentence which places "(user manager)" > > after "guest profile" as if the user manager is another name for the guest > > profile. It's like reading a sentence like "I went to the store and bought > two > > cucumbers (football team)." "football team" is mystifying there. > > > > So I meant, clarify how the user manager relates to what's returned. > > Acknowledged. > > > > > This is related to the issue where it seems strange that we return the guest > > profile if we intend to show the user manager to begin with. > > Looks like we should return a structure with a enum and optionally a profile, in > which the enum class looks like OpenAtStartup {PROFILE, USER_MANAGER}. Returning > a profile to request the user manager is confusing, and it blocks the > possibility to open the real guest profile. (If the user manager fails to show, > I believe that we should fallback to the real guest profile.) > > It's likely that we should start a new issue for this. (But well, this is > already the third issue that we have come up with for one single issue.) > > Mike: > Do you know what was happening with this code in the past? Why do we return the > guest profile to denote the user manager? Was there any special reason in the > design? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
profiles changes and related behavior LGTM. I'll let pkasting@ come back in for the startup flows.
https://codereview.chromium.org/2047483003/diff/200001/base/test/test_file_ut... File base/test/test_file_util_win.cc (right): https://codereview.chromium.org/2047483003/diff/200001/base/test/test_file_ut... base/test/test_file_util_win.cc:78: // Deny |permission| on the file |path|, for the current user. All my comments in https://codereview.chromium.org/2061593002/diff/80001/base/test/test_file_uti... apply. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/profile... File chrome/browser/profiles/profiles_state.cc (right): https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/profile... chrome/browser/profiles/profiles_state.cc:220: // --profile-directory switch. Nit: "|has_entry| may be false when the user specified a profile on the command line." https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:900: // be opened for the guest profile. The initialization of guest profile is Do you mean "for the active profile"? I'm not sure how switching to guest would prevent windows being opened for guest. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:901: // possible to fail. Nit: is possible to -> may But what is the import of the guest profile failing to init? https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:904: auto& storage = profile_manager->GetProfileAttributesStorage(); Nit: Either use const auto& or auto* (2 places) https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:910: entry->IsSigninRequired())) { Nit: Reverse conditional and early-return |profile|, so the rest can be unindented https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:912: // need to know that the system profile (where the user manager lives on) Nit: where...on -> in which https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:918: profile = nullptr; Nit: Once early-return suggestion above is followed, can become: return profile_manager->GetProfile(ProfileManager::GetSystemProfilePath()) ? profile_manager->GetProfile(ProfileManager::GetGuestProfilePath()) : nullptr; https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:929: // profiles is expected to fail. So only existing profiles are attempted for If "directory creation" is the only reason for init failure, how could existing profiles (which already have directories) ever fail? https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:950: Profile* system_profile = Nit: Can inline into next statement https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:184: // the profile return by GetStartupProfilePath, or the guest profile if the Nit: returned https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:186: // user manager. Returns null if the above profile cannot be opened. In case of Why does returning the guest profile denote opening the user manager? Why not return some sort of code or boolean indicating whether to show the user manager? Or do we have to use the guest profile behind the scenes while the user manager is visible (and if so why)? https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:187: // user manager, returns null if either the guest profile and the system profile Nit: Do you mean "neither/nor...can be opened"? How does the "system profile" (what is that) factor into everything? https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:192: // Returns the a fallback profile that should be loaded on process startup. Nit: extra a Explain what a fallback profile is. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:193: // Possible to return null, which means no profile (including user manager) can The user manager isn't a profile, so this sentence is a bit confusing. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc (right): https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:85: expected_basepaths.cbegin())) { Nit: If you use the four-arg version of is_permutation(), you can omit the explicit size check. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:136: return base::DenyFilePermission(user_data_dir, FILE_ADD_SUBDIRECTORY); Nit: return PathService::Get(chrome::DIR_USER_DATA, &user_data_dir) && base::DenyFilePermission(user_data_dir, FILE_ADD_SUBDIRECTORY); https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:317: // WaitForAndHideUserManager(); Uncomment or remove
https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc (right): https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:136: return base::DenyFilePermission(user_data_dir, FILE_ADD_SUBDIRECTORY); I prefer WC's version to this. Please keep it as-is. This is not covered by the style guide and there's isn't a clear prevailing style or clearly more readable approach, so people should feel free to write what they think is most clear.
lgtm https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:414: profiles::IsMultipleProfilesEnabled() && This same condition is used near the top of this function. Let's move this variable definition to the top and replace the condition with your variable so we don't have to check HasSwitch twice for the same thing. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:898: // locked, then we should show the user manager instead. By switching the The old comment had the wording "signed out". You added "locked" but I think we should preserve the signed out wording also since I think that will actually be the common case when we hit this code path.
https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc (right): https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:136: return base::DenyFilePermission(user_data_dir, FILE_ADD_SUBDIRECTORY); On 2016/07/01 22:21:53, brettw (plz ping after 24h) wrote: > I prefer WC's version to this. Please keep it as-is. This is not covered by the > style guide and there's isn't a clear prevailing style or clearly more readable > approach, so people should feel free to write what they think is most clear. I'm sorry for not being clear. My suggestion is merely a suggestion, not a mandate. I find the form I suggest clearer and easier to read. Brett finds the other form clearer and easier to read. Please use whichever form you prefer.
Patchset #10 (id:220001) has been deleted
New patch uploaded. Peter Kasting: PTAL. Note that the code for base/test/* is in the other CL. https://codereview.chromium.org/2047483003/diff/200001/base/test/test_file_ut... File base/test/test_file_util_win.cc (right): https://codereview.chromium.org/2047483003/diff/200001/base/test/test_file_ut... base/test/test_file_util_win.cc:78: // Deny |permission| on the file |path|, for the current user. On 2016/07/01 00:53:23, Peter Kasting wrote: > All my comments in > https://codereview.chromium.org/2061593002/diff/80001/base/test/test_file_uti... > apply. I've updated the code in the other CL. PTAL. If that's good I'll move it here. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:414: profiles::IsMultipleProfilesEnabled() && On 2016/07/01 23:13:22, brettw (plz ping after 24h) wrote: > This same condition is used near the top of this function. Let's move this > variable definition to the top and replace the condition with your variable so > we don't have to check HasSwitch twice for the same thing. Done. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/profile... File chrome/browser/profiles/profiles_state.cc (right): https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/profile... chrome/browser/profiles/profiles_state.cc:220: // --profile-directory switch. On 2016/07/01 00:53:23, Peter Kasting wrote: > Nit: "|has_entry| may be false when the user specified a profile on the command > line." Done. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:898: // locked, then we should show the user manager instead. By switching the On 2016/07/01 23:13:22, brettw (plz ping after 24h) wrote: > The old comment had the wording "signed out". You added "locked" but I think we > should preserve the signed out wording also since I think that will actually be > the common case when we hit this code path. Not sure you've read Mike's comment in https://codereview.chromium.org/2047483003/diff/180001/chrome/browser/ui/star.... Anyway the exact action in the UI to "sign out" is called "Exit and Childlock". I do not remember exactly how to access that function though. Here are the steps if I remember correctly: 1. Create a profile linked to gmail. 2. Create supervised accounts under the profile in (1). 3. Left click the profile button in the title bar. 4. Select "Exit and Childlock". https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:900: // be opened for the guest profile. The initialization of guest profile is On 2016/07/01 00:53:23, Peter Kasting wrote: > Do you mean "for the active profile"? I'm not sure how switching to guest would > prevent windows being opened for guest. Oh... that sentence was not written by me. It was in the unpatched code instead. BTW, I suggest modifying the sentence to "This is done by setting the profile to the guest profile." https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:901: // possible to fail. On 2016/07/01 00:53:23, Peter Kasting wrote: > Nit: is possible to -> may > > But what is the import of the guest profile failing to init? By design, I think it is not important at all. However, currently if the guest profile fail to initialize then we're returning a null pointer, so the failure is currently significant. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:904: auto& storage = profile_manager->GetProfileAttributesStorage(); On 2016/07/01 00:53:23, Peter Kasting wrote: > Nit: Either use const auto& or auto* (2 places) Not sure here. The declaration of GetProfileAttributesStroage() is currently a ProfileAttributesStroage&. And I've heard that the rationale was "this function cannot return a null pointer". I know that this is contradicting from google c++ guidelines, so sorry for the confusion. There's even a very interesting workaround in the testing code: https://cs.chromium.org/chromium/src/chrome/test/base/testing_profile_manager... WDYT? https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:910: entry->IsSigninRequired())) { On 2016/07/01 00:53:23, Peter Kasting wrote: > Nit: Reverse conditional and early-return |profile|, so the rest can be > unindented Done. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:912: // need to know that the system profile (where the user manager lives on) On 2016/07/01 00:53:23, Peter Kasting wrote: > Nit: where...on -> in which Done. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:918: profile = nullptr; On 2016/07/01 00:53:23, Peter Kasting wrote: > Nit: Once early-return suggestion above is followed, can become: > > return profile_manager->GetProfile(ProfileManager::GetSystemProfilePath()) ? > profile_manager->GetProfile(ProfileManager::GetGuestProfilePath()) : > nullptr; Done. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:929: // profiles is expected to fail. So only existing profiles are attempted for On 2016/07/01 00:53:23, Peter Kasting wrote: > If "directory creation" is the only reason for init failure, how could existing > profiles (which already have directories) ever fail? Appears that the only way is to have some users delete the profile, and have the user-data-directory set to "deny creating new directories". Sadly this is discovered not the cause of 614753. (Frankly I have no clue how the crash happens.) BTW, if we disable writing rights on the directory, chromium simply crash some time after startup. This is not related to this CL though. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:950: Profile* system_profile = On 2016/07/01 00:53:23, Peter Kasting wrote: > Nit: Can inline into next statement I'd rather not to change. With the extra braces in the if statement, we don't reduce any lines of code by the change. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:184: // the profile return by GetStartupProfilePath, or the guest profile if the On 2016/07/01 00:53:23, Peter Kasting wrote: > Nit: returned Done. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:186: // user manager. Returns null if the above profile cannot be opened. In case of On 2016/07/01 00:53:23, Peter Kasting wrote: > Why does returning the guest profile denote opening the user manager? Why not > return some sort of code or boolean indicating whether to show the user manager? I suggest a structure with an enum and a Profile*. BTW, let's start a separate issue for this. I need to grok the different callsites before making a CL that does not break things. > Or do we have to use the guest profile behind the scenes while the user manager > is visible (and if so why)? No. That's why it's very strange. And the Mac code seems NOT to follow this tradition of returning the guest profile at all. (I'm not 100% sure about this though.) https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:187: // user manager, returns null if either the guest profile and the system profile On 2016/07/01 00:53:24, Peter Kasting wrote: > Nit: Do you mean "neither/nor...can be opened"? How does the "system profile" > (what is that) factor into everything? I do really mean "either". If the guest profile cannot be opened we have nothing to return (until we solved the issue above). If if the system profile cannot be opened we cannot load the user manager. BTW, I'd rather not to explain about the system profile at this stage, because the most confusing part is the guest profile. (And you're commented in several places about the guest profile.) When/if the issue above is finally solved, then we don't need to explain at all. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:192: // Returns the a fallback profile that should be loaded on process startup. On 2016/07/01 00:53:24, Peter Kasting wrote: > Nit: extra a > > Explain what a fallback profile is. Done. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:193: // Possible to return null, which means no profile (including user manager) can On 2016/07/01 00:53:24, Peter Kasting wrote: > The user manager isn't a profile, so this sentence is a bit confusing. Revised the comment. PTAL. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc (right): https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:85: expected_basepaths.cbegin())) { On 2016/07/01 00:53:24, Peter Kasting wrote: > Nit: If you use the four-arg version of is_permutation(), you can omit the > explicit size check. The four-arg version is C++14, so it is not acceptable in Chromium. Anyway, I've made this mistake several months ago with Windows (sadly Windows), and then... trybots failed on compile. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:136: return base::DenyFilePermission(user_data_dir, FILE_ADD_SUBDIRECTORY); On 2016/07/02 00:15:42, Peter Kasting wrote: > On 2016/07/01 22:21:53, brettw (plz ping after 24h) wrote: > > I prefer WC's version to this. Please keep it as-is. This is not covered by > the > > style guide and there's isn't a clear prevailing style or clearly more > readable > > approach, so people should feel free to write what they think is most clear. > > I'm sorry for not being clear. My suggestion is merely a suggestion, not a > mandate. > > I find the form I suggest clearer and easier to read. Brett finds the other > form clearer and easier to read. Please use whichever form you prefer. To me both forms are readable. Which one is easier to read depends on prior experience. BTW, I'll follow Peter's suggestion because the file is owned by him. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:317: // WaitForAndHideUserManager(); On 2016/07/01 00:53:24, Peter Kasting wrote: > Uncomment or remove Done. Sorry for leaving testing code here.
The CQ bit was checked by lwchkg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2047483003/diff/200001/base/test/test_file_ut... File base/test/test_file_util_win.cc (right): https://codereview.chromium.org/2047483003/diff/200001/base/test/test_file_ut... base/test/test_file_util_win.cc:78: // Deny |permission| on the file |path|, for the current user. On 2016/07/07 17:01:53, WC Leung wrote: > On 2016/07/01 00:53:23, Peter Kasting wrote: > > All my comments in > > > https://codereview.chromium.org/2061593002/diff/80001/base/test/test_file_uti... > > apply. > > I've updated the code in the other CL. PTAL. If that's good I'll move it here. I reviewed that CL now. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:904: auto& storage = profile_manager->GetProfileAttributesStorage(); On 2016/07/07 17:01:53, WC Leung wrote: > On 2016/07/01 00:53:23, Peter Kasting wrote: > > Nit: Either use const auto& or auto* (2 places) > > Not sure here. > > The declaration of GetProfileAttributesStroage() is currently a > ProfileAttributesStroage&. And I've heard that the rationale was "this function > cannot return a null pointer". This should be changed. If callers need a non-const object, it should return a pointer, else it should return a const ref. If it returns a pointer, the API comment can note it never returns null. Plenty of code in our codebase returns pointers from functions that never return null. It is OK to fix that in a separate CL, but until that's done I would use "auto* storage = &..." here. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:929: // profiles is expected to fail. So only existing profiles are attempted for On 2016/07/07 17:01:53, WC Leung wrote: > On 2016/07/01 00:53:23, Peter Kasting wrote: > > If "directory creation" is the only reason for init failure, how could > existing > > profiles (which already have directories) ever fail? > > Appears that the only way is to have some users delete the profile, and have the > user-data-directory set to "deny creating new directories". I don't understand. Wouldn't these still be cases where the profiles would be deleted and thus not in the "existing profiles" list? Or is the "existing profiles" list sourced from somewhere else, and may include things that have since been deleted on disk? If so it would be nice if the comments could make this clearer. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:950: Profile* system_profile = On 2016/07/07 17:01:53, WC Leung wrote: > On 2016/07/01 00:53:23, Peter Kasting wrote: > > Nit: Can inline into next statement > > I'd rather not to change. With the extra braces in the if statement, we don't > reduce any lines of code by the change. Really?: if (guest_profile && profile_manager->GetProfile(ProfileManager::GetSystemProfilePath()) return guest_profile; Not sure what you mean by extra braces unless you mean to add braces on the above, which wouldn't be necessary, since it has a one-line body (the length of the conditional is irrelevant when making this determination). To be clear, it's fine to leave it the way you have it too. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:186: // user manager. Returns null if the above profile cannot be opened. In case of On 2016/07/07 17:01:54, WC Leung wrote: > On 2016/07/01 00:53:23, Peter Kasting wrote: > > Why does returning the guest profile denote opening the user manager? Why not > > return some sort of code or boolean indicating whether to show the user > manager? > > I suggest a structure with an enum and a Profile*. BTW, let's start a separate > issue for this. I need to grok the different callsites before making a CL that > does not break things. > > > Or do we have to use the guest profile behind the scenes while the user > manager > > is visible (and if so why)? > > No. That's why it's very strange. And the Mac code seems NOT to follow this > tradition of returning the guest profile at all. (I'm not 100% sure about this > though.) Hmm. Sounds like this does need cleanup, and you'd rather do it in another CL, which I'm OK with. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:187: // user manager, returns null if either the guest profile and the system profile On 2016/07/07 17:01:54, WC Leung wrote: > On 2016/07/01 00:53:24, Peter Kasting wrote: > > Nit: Do you mean "neither/nor...can be opened"? How does the "system profile" > > (what is that) factor into everything? > > I do really mean "either". Then I think you meant to use "either...or" instead of "either...and". https://codereview.chromium.org/2047483003/diff/240001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2047483003/diff/240001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:900: // possible to fail. Nit: Let's try to keep the bits about the guest profile and it failing init only in the comment on the next block. Here I would just say: If there is no entry in profile attributes storage, the profile is deleted, and we should show the user manager. Also, when using --new-profile-management, if the profile is locked we should show the user manager as well. When neither of these is true, we can safely start up with |profile|. https://codereview.chromium.org/2047483003/diff/240001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:914: // denote failure. Nit: How about: We want the caller to show the user manager. To indicate this, return the guest profile. However, we can only do this if the system profile (where the user manager lives) also exists (or is creatable). https://codereview.chromium.org/2047483003/diff/240001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/2047483003/diff/240001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:195: // opened. The fallback is used if GetStartupProfile() returns null. Nit: I think this has all the necessary info now, but it's stated in a roundabout way. How about: Returns the profile that should be loaded on process startup when GetStartupProfile() returns null. As with GetStartupProfile(), returning the guest profile means the caller should open the user manager. This may return null if no profile nor the user manager can be opened. ...Though this makes me wonder. If this is only used when GetStartupProfile() is null, how can this ever return the guest profile? Wouldn't GetStartupProfile() being null imply the user manager can't be opened?
https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:904: auto& storage = profile_manager->GetProfileAttributesStorage(); On 2016/07/11 02:58:58, Peter Kasting (OOO til Jul 18) wrote: > On 2016/07/07 17:01:53, WC Leung wrote: > > On 2016/07/01 00:53:23, Peter Kasting wrote: > > > Nit: Either use const auto& or auto* (2 places) > > > > Not sure here. > > > > The declaration of GetProfileAttributesStroage() is currently a > > ProfileAttributesStroage&. And I've heard that the rationale was "this > function > > cannot return a null pointer". > > This should be changed. If callers need a non-const object, it should return a > pointer, else it should return a const ref. If it returns a pointer, the API > comment can note it never returns null. Plenty of code in our codebase returns > pointers from functions that never return null. > > It is OK to fix that in a separate CL, but until that's done I would use "auto* > storage = &..." here. Changing the return type of this function would be a massive change, whose discussion and implementation would definitely not happen in this CL.
lwchkg@chromium.org changed reviewers: + phajdan.jr@chromium.org
New patch uploaded. Peter: PTAL. Paweł: Some code in base/test/* is copied from https://codereview.chromium.org/2061593002/ . PTAL. https://codereview.chromium.org/2047483003/diff/200001/base/test/test_file_ut... File base/test/test_file_util_win.cc (right): https://codereview.chromium.org/2047483003/diff/200001/base/test/test_file_ut... base/test/test_file_util_win.cc:78: // Deny |permission| on the file |path|, for the current user. On 2016/07/11 02:58:58, Peter Kasting (OOO til Jul 18) wrote: > On 2016/07/07 17:01:53, WC Leung wrote: > > On 2016/07/01 00:53:23, Peter Kasting wrote: > > > All my comments in > > > > > > https://codereview.chromium.org/2061593002/diff/80001/base/test/test_file_uti... > > > apply. > > > > I've updated the code in the other CL. PTAL. If that's good I'll move it here. > > I reviewed that CL now. Acknowledged. Now I copied the code back to this CL. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:904: auto& storage = profile_manager->GetProfileAttributesStorage(); On 2016/07/11 10:01:08, Mike Lerman wrote: > On 2016/07/11 02:58:58, Peter Kasting (OOO til Jul 18) wrote: > > On 2016/07/07 17:01:53, WC Leung wrote: > > > On 2016/07/01 00:53:23, Peter Kasting wrote: > > > > Nit: Either use const auto& or auto* (2 places) > > > > > > Not sure here. > > > > > > The declaration of GetProfileAttributesStroage() is currently a > > > ProfileAttributesStroage&. And I've heard that the rationale was "this > > function > > > cannot return a null pointer". > > > > This should be changed. If callers need a non-const object, it should return > a > > pointer, else it should return a const ref. If it returns a pointer, the API > > comment can note it never returns null. Plenty of code in our codebase > returns > > pointers from functions that never return null. > > > > It is OK to fix that in a separate CL, but until that's done I would use > "auto* > > storage = &..." here. Done. > Changing the return type of this function would be a massive change, whose > discussion and implementation would definitely not happen in this CL. I see. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:929: // profiles is expected to fail. So only existing profiles are attempted for On 2016/07/11 02:58:59, Peter Kasting (OOO til Jul 18) wrote: > On 2016/07/07 17:01:53, WC Leung wrote: > > On 2016/07/01 00:53:23, Peter Kasting wrote: > > > If "directory creation" is the only reason for init failure, how could > > existing > > > profiles (which already have directories) ever fail? > > > > Appears that the only way is to have some users delete the profile, and have > the > > user-data-directory set to "deny creating new directories". > > I don't understand. Wouldn't these still be cases where the profiles would be > deleted and thus not in the "existing profiles" list? In this case we show the user manager. See https://codereview.chromium.org/2047483003/diff/100001/chrome/browser/ui/star... for the discussion. > Or is the "existing > profiles" list sourced from somewhere else, and may include things that have > since been deleted on disk? If so it would be nice if the comments could make > this clearer. And in this case we try to create the profile. Anyway, if profile creation fails GetFallbackStartupProfile() may be called. BTW, both cases should be handled in GetStartupProfile() instead of GetFallbackStartupProfile(). And anyway, the comment just state that we do not attempt to create new profiles because that is expected to fail. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:950: Profile* system_profile = On 2016/07/11 02:58:58, Peter Kasting (OOO til Jul 18) wrote: > On 2016/07/07 17:01:53, WC Leung wrote: > > On 2016/07/01 00:53:23, Peter Kasting wrote: > > > Nit: Can inline into next statement > > > > I'd rather not to change. With the extra braces in the if statement, we don't > > reduce any lines of code by the change. > > Really?: > > if (guest_profile && > profile_manager->GetProfile(ProfileManager::GetSystemProfilePath()) > return guest_profile; I've been asked to add the braces when the conditions are longer than one line. Anyway I'm not sure about the rationale: the google c++ style guide did not discuss about this case. Not sure whether this is discussed in the internal guide though. > > Not sure what you mean by extra braces unless you mean to add braces on the > above, which wouldn't be necessary, since it has a one-line body (the length of > the conditional is irrelevant when making this determination). > > To be clear, it's fine to leave it the way you have it too. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:187: // user manager, returns null if either the guest profile and the system profile On 2016/07/11 02:58:59, Peter Kasting (OOO til Jul 18) wrote: > On 2016/07/07 17:01:54, WC Leung wrote: > > On 2016/07/01 00:53:24, Peter Kasting wrote: > > > Nit: Do you mean "neither/nor...can be opened"? How does the "system > profile" > > > (what is that) factor into everything? > > > > I do really mean "either". > > Then I think you meant to use "either...or" instead of "either...and". Done. Sorry for this stupid mistake. https://codereview.chromium.org/2047483003/diff/240001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2047483003/diff/240001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:900: // possible to fail. On 2016/07/11 02:58:59, Peter Kasting (OOO til Jul 18) wrote: > Nit: Let's try to keep the bits about the guest profile and it failing init only > in the comment on the next block. Here I would just say: > > If there is no entry in profile attributes storage, the profile is deleted, and > we should show the user manager. Also, when using --new-profile-management, if > the profile is locked we should show the user manager as well. When neither of > these is true, we can safely start up with |profile|. Sounds good. https://codereview.chromium.org/2047483003/diff/240001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:914: // denote failure. On 2016/07/11 02:58:59, Peter Kasting (OOO til Jul 18) wrote: > Nit: How about: > > We want the caller to show the user manager. To indicate this, return the guest > profile. However, we can only do this if the system profile (where the user > manager lives) also exists (or is creatable). Sounds good. Thanks. https://codereview.chromium.org/2047483003/diff/240001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/2047483003/diff/240001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:195: // opened. The fallback is used if GetStartupProfile() returns null. On 2016/07/11 02:58:59, Peter Kasting (OOO til Jul 18) wrote: > Nit: I think this has all the necessary info now, but it's stated in a > roundabout way. How about: > > Returns the profile that should be loaded on process startup when > GetStartupProfile() returns null. As with GetStartupProfile(), returning the > guest profile means the caller should open the user manager. This may return > null if no profile nor the user manager can be opened. Done. > > ...Though this makes me wonder. If this is only used when GetStartupProfile() > is null, how can this ever return the guest profile? Wouldn't > GetStartupProfile() being null imply the user manager can't be opened? The normal flow is like this: (1) last active profile / selected profile -> (2) user manager if profile does not exist or profile is locked (does not exist means it is not in profile attribute storage) The fallback flow is like this: (0) normal flow fails -> (1) any of the last opened profile which is not locked -> (2) user manager -> (3) any profile which is not locked -> (4) give up If (1) is selected in the normal flow, but profile initialization fails, then GetStartupProfile() returns null. In this case it is possible can have the user manager (2) selected in the fallback flow.
New patch really uploaded. (Said "uploaded" yesterday, but sadly nothing was uploaded except for base/test/*.)
LGTM https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:950: Profile* system_profile = On 2016/07/18 17:56:18, WC Leung wrote: > On 2016/07/11 02:58:58, Peter Kasting (OOO til Jul 18) wrote: > > On 2016/07/07 17:01:53, WC Leung wrote: > > > On 2016/07/01 00:53:23, Peter Kasting wrote: > > > > Nit: Can inline into next statement > > > > > > I'd rather not to change. With the extra braces in the if statement, we > don't > > > reduce any lines of code by the change. > > > > Really?: > > > > if (guest_profile && > > profile_manager->GetProfile(ProfileManager::GetSystemProfilePath()) > > return guest_profile; > > I've been asked to add the braces when the conditions are longer than one line. > Anyway I'm not sure about the rationale: the google c++ style guide did not > discuss about this case. Not sure whether this is discussed in the internal > guide though. Both style guides are mute (the Chrome one intentionally so) on this issue. So both forms are legal. It is thus up to author + reviewer discretion whether to use one or the other. Both forms are common in the codebase. While my personal preference is to omit the braces, use your judgment on what is readable. https://codereview.chromium.org/2047483003/diff/240001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/2047483003/diff/240001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:195: // opened. The fallback is used if GetStartupProfile() returns null. On 2016/07/18 17:56:18, WC Leung wrote: > On 2016/07/11 02:58:59, Peter Kasting (OOO til Jul 18) wrote: > > Nit: I think this has all the necessary info now, but it's stated in a > > roundabout way. How about: > > > > Returns the profile that should be loaded on process startup when > > GetStartupProfile() returns null. As with GetStartupProfile(), returning the > > guest profile means the caller should open the user manager. This may return > > null if no profile nor the user manager can be opened. > > Done. > > > > ...Though this makes me wonder. If this is only used when GetStartupProfile() > > is null, how can this ever return the guest profile? Wouldn't > > GetStartupProfile() being null imply the user manager can't be opened? > > The normal flow is like this: > (1) last active profile / selected profile -> > (2) user manager if profile does not exist or profile is locked > (does not exist means it is not in profile attribute storage) > > The fallback flow is like this: > (0) normal flow fails -> > (1) any of the last opened profile which is not locked -> > (2) user manager -> > (3) any profile which is not locked -> > (4) give up > > If (1) is selected in the normal flow, but profile initialization fails, then > GetStartupProfile() returns null. In this case it is possible can have the user > manager (2) selected in the fallback flow. So, is this a case where profile_manager->GetProfile(profile_path) returns null, but storage.GetProfileAttributesWithPath(profile_path, &entry) returns true? https://codereview.chromium.org/2047483003/diff/300001/base/test/test_file_ut... File base/test/test_file_util.h (right): https://codereview.chromium.org/2047483003/diff/300001/base/test/test_file_ut... base/test/test_file_util.h:47: // Deny |permission| on the file |path|, for the current user. Nit: Might want to say something like "|permission| is a set of ACCESS_MASK flags; see the |grfAccessPermissions| field of https://msdn.microsoft.com/en-us/library/windows/desktop/aa446627(v=vs.85).aspx ." or similar. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:920: // The only known reason for profiles to fail initialization is unable to Nit: unable -> being unable https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:927: // opened profiles can initialize successfully. Nit: "The last used profile could not be initialized, so see if any other last opened profiles can be initialized successfully." https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:930: auto& storage = profile_manager->GetProfileAttributesStorage(); Nit: Use auto* storage = &... https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:931: for (Profile* last_opened_profile : last_opened_profiles) { Nit: Avoids extra temp: for (Profile* profile : ProfileManager::GetLastOpenedProfiles()) { https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:941: // Show the user manager if this is successful. Nit: "Couldn't initialize any last opened profiles. Try to show the user manager, which requires successful initialization of the guest and system profiles." https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:949: // If still fails, try to open any profile that is not locked. Nit: "Couldn't show the user manager either. Try to open..." https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:952: for (ProfileAttributesEntry* entry : entries) { Nit: Avoids extra temp: for (ProfileAttributesEntry* entry : storage.GetAllProfilesAttributes()) { https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:187: // user manager, returns null if either the guest profile or the system profile Nit: user -> opening the user https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:195: // null if no profile nor the user manager can be opened. Nit: no profile nor the user manager -> neither the user manager nor any profile https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc (right): https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. No (c) (see https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... ). https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:30: const ProfileManager::CreateCallback kOnProfileSwitchDoNothing; Nit: I'm not sure this buys much over just inlining "ProfileManager::CreateCallback()" at the one usage below. If you really think this is a readability win, prefer to declare locally just above the use, in keeping with the suggestions in http://google.github.io/styleguide/cppguide.html#Local_Variables . https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:52: void CloseBrowsersSuccessCallback(const base::Closure& quit_closure, Nit: Maybe OnCloseAllBrowsersSucceeded()? https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:67: // terminated by OnUnblockOnProfileCreation when the profile is created. Nit: Extra On https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:75: BrowserList* browser_list = BrowserList::GetInstance(); Nit: Just inline this below https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:77: Nit: Unnecessary blank line https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:95: // If the user manager is shown when the browser process die, a crash will Nit: "Waits for the user manager to appear." Then, in the test body, above the Hide() call: "If the user manager is still showing when the browser process dies, ____ will crash. So hide it before the caller ends the test." (I'm not sure what is actually crashing here, fill in the blank.) https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:98: void WaitForAndHideUserManager() { Nit: Maybe name this ExpectUserManagerShown() since from the caller perspective that's the critical bit (hiding the user manager afterwards is more like an implementation detail). https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:140: // importantly remove directories before the browser process starts. How come we can't do the steps in these functions inside the PRE_ functions instead, and collapse each test from three parts to two? https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:221: // Most of the tests below has three sections: Nit: has -> have Somewhere in this comment you should probably also explain why this functionality can't all be in one function for each test (like is commonly done). https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:225: // (3) The main test, where the real test is taken. Nit: Just "The test itself." is probably fine https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:248: // missing, it should fallback to any last opened profiles. Nit: When using "fallback" as a verb, spell it "fall back". "fallback" is a noun, or adjective. (Several places) https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:266: // LastUsedProfileFallbackToUserManager : If all last used profile is Nit: profile is -> profiles are https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:267: // missing, it should fallback to user manager. To open the user manager, both Nit: it should fallback to -> fall back to the https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:268: // the guesst profile and the system profile must be creatable. Nit: guest https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:301: // Create the guest profile path only. The system profile cannot be created. Nit: "Create the guest profile path, but not the system profile one. This will make it impossible to create the system profile once the permissions are locked down during setup." https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:318: // guest profile cannot be opened, fallback to any non-signin profile. Nit: non-signin -> not-signed-in https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:339: // an appropriate exit code (0). Nit: This doesn't check the browser exit code, so I wouldn't talk about it. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:353: // fallback to in this test, chromium should not start. Nit: "Profiles that are locked should never be initialized. Since there are no unlocked profiles, the browser should not start." https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:356: // Sign out the default profile and wait for the user manager. Why is the user manager appearing during the PRE_ test? Isn't this just setup before the browser starts? https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:373: // profile is selected by --profile-directory switch. Chromium should not start Nit: by -> by the; Chromium -> The browser https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:374: // if the specified profile could not initialize. Nit: initialize -> be initialized https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:397: // attempt to open the profile. Instead shows the user manager. Nit: "If the profile's entry in ProfileAttributesStorage is missing, the profile is deleted. The browser should not attempt to open the profile, but should show the user manager instead."
LGTM
The CQ bit was checked by lwchkg@chromium.org to run a CQ dry run
Patchset #15 (id:340001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patches uploaded to address Peter's comments and fix the clang compile failure. Peter: PTAL. https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2047483003/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:950: Profile* system_profile = On 2016/07/19 21:56:06, Peter Kasting wrote: > On 2016/07/18 17:56:18, WC Leung wrote: > > On 2016/07/11 02:58:58, Peter Kasting (OOO til Jul 18) wrote: > > > On 2016/07/07 17:01:53, WC Leung wrote: > > > > On 2016/07/01 00:53:23, Peter Kasting wrote: > > > > > Nit: Can inline into next statement > > > > > > > > I'd rather not to change. With the extra braces in the if statement, we > > don't > > > > reduce any lines of code by the change. > > > > > > Really?: > > > > > > if (guest_profile && > > > profile_manager->GetProfile(ProfileManager::GetSystemProfilePath()) > > > return guest_profile; > > > > I've been asked to add the braces when the conditions are longer than one > line. > > Anyway I'm not sure about the rationale: the google c++ style guide did not > > discuss about this case. Not sure whether this is discussed in the internal > > guide though. > > Both style guides are mute (the Chrome one intentionally so) on this issue. So > both forms are legal. It is thus up to author + reviewer discretion whether to > use one or the other. Both forms are common in the codebase. While my personal > preference is to omit the braces, use your judgment on what is readable. I see. Thanks! https://codereview.chromium.org/2047483003/diff/240001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/2047483003/diff/240001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:195: // opened. The fallback is used if GetStartupProfile() returns null. On 2016/07/19 21:56:06, Peter Kasting wrote: > On 2016/07/18 17:56:18, WC Leung wrote: > > On 2016/07/11 02:58:59, Peter Kasting (OOO til Jul 18) wrote: > > > Nit: I think this has all the necessary info now, but it's stated in a > > > roundabout way. How about: > > > > > > Returns the profile that should be loaded on process startup when > > > GetStartupProfile() returns null. As with GetStartupProfile(), returning > the > > > guest profile means the caller should open the user manager. This may > return > > > null if no profile nor the user manager can be opened. > > > > Done. > > > > > > ...Though this makes me wonder. If this is only used when > GetStartupProfile() > > > is null, how can this ever return the guest profile? Wouldn't > > > GetStartupProfile() being null imply the user manager can't be opened? > > > > The normal flow is like this: > > (1) last active profile / selected profile -> > > (2) user manager if profile does not exist or profile is locked > > (does not exist means it is not in profile attribute storage) > > > > The fallback flow is like this: > > (0) normal flow fails -> > > (1) any of the last opened profile which is not locked -> > > (2) user manager -> > > (3) any profile which is not locked -> > > (4) give up > > > > If (1) is selected in the normal flow, but profile initialization fails, then > > GetStartupProfile() returns null. In this case it is possible can have the > user > > manager (2) selected in the fallback flow. > > So, is this a case where profile_manager->GetProfile(profile_path) returns null, > but storage.GetProfileAttributesWithPath(profile_path, &entry) returns true? Yes. https://codereview.chromium.org/2047483003/diff/300001/base/test/test_file_ut... File base/test/test_file_util.h (right): https://codereview.chromium.org/2047483003/diff/300001/base/test/test_file_ut... base/test/test_file_util.h:47: // Deny |permission| on the file |path|, for the current user. On 2016/07/19 21:56:06, Peter Kasting wrote: > Nit: Might want to say something like "|permission| is a set of ACCESS_MASK > flags; see the |grfAccessPermissions| field of > https://msdn.microsoft.com/en-us/library/windows/desktop/aa446627(v=vs.85).aspx > ." or similar. Does MSDN urls change? BTW, I'm negative about adding a specific URL because none of them deliver the required information. Adding a statement referring them to ACCESS_MARK and "file and directory access right constants" may help though. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:920: // The only known reason for profiles to fail initialization is unable to On 2016/07/19 21:56:07, Peter Kasting wrote: > Nit: unable -> being unable Done. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:927: // opened profiles can initialize successfully. On 2016/07/19 21:56:06, Peter Kasting wrote: > Nit: "The last used profile could not be initialized, so see if any other last > opened profiles can be initialized successfully." Done. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:930: auto& storage = profile_manager->GetProfileAttributesStorage(); On 2016/07/19 21:56:07, Peter Kasting wrote: > Nit: Use auto* storage = &... Oh my bad... should have done this before. Why I didn't make it in the CL? Done anyway. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:931: for (Profile* last_opened_profile : last_opened_profiles) { On 2016/07/19 21:56:06, Peter Kasting wrote: > Nit: Avoids extra temp: > > for (Profile* profile : ProfileManager::GetLastOpenedProfiles()) { Done. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:941: // Show the user manager if this is successful. On 2016/07/19 21:56:07, Peter Kasting wrote: > Nit: "Couldn't initialize any last opened profiles. Try to show the user > manager, which requires successful initialization of the guest and system > profiles." Done. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:949: // If still fails, try to open any profile that is not locked. On 2016/07/19 21:56:07, Peter Kasting wrote: > Nit: "Couldn't show the user manager either. Try to open..." Done. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:952: for (ProfileAttributesEntry* entry : entries) { On 2016/07/19 21:56:07, Peter Kasting wrote: > Nit: Avoids extra temp: > > for (ProfileAttributesEntry* entry : storage.GetAllProfilesAttributes()) { Done. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:187: // user manager, returns null if either the guest profile or the system profile On 2016/07/19 21:56:07, Peter Kasting wrote: > Nit: user -> opening the user Done. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:195: // null if no profile nor the user manager can be opened. On 2016/07/19 21:56:07, Peter Kasting wrote: > Nit: no profile nor the user manager -> neither the user manager nor any profile Done. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc (right): https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/07/19 21:56:07, Peter Kasting wrote: > No (c) (see > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > ). Thanks for noting. Where did I copy the notice??? https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:30: const ProfileManager::CreateCallback kOnProfileSwitchDoNothing; I wanted to make it consistent with profile_manager_browsertest.cc. See https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_manager_... BTW, this is only used once in this file, so inlining sounds good. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:52: void CloseBrowsersSuccessCallback(const base::Closure& quit_closure, On 2016/07/19 21:56:08, Peter Kasting wrote: > Nit: Maybe OnCloseAllBrowsersSucceeded()? Sounds good. Done. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:67: // terminated by OnUnblockOnProfileCreation when the profile is created. On 2016/07/19 21:56:08, Peter Kasting wrote: > Nit: Extra On Done. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:75: BrowserList* browser_list = BrowserList::GetInstance(); On 2016/07/19 21:56:08, Peter Kasting wrote: > Nit: Just inline this below Done. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:77: On 2016/07/19 21:56:08, Peter Kasting wrote: > Nit: Unnecessary blank line Done. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:95: // If the user manager is shown when the browser process die, a crash will On 2016/07/19 21:56:08, Peter Kasting wrote: > Nit: "Waits for the user manager to appear." > > Then, in the test body, above the Hide() call: "If the user manager is still > showing when the browser process dies, ____ will crash. So hide it before the > caller ends the test." (I'm not sure what is actually crashing here, fill in > the blank.) I have no idea what is crashing. But since UserManager is a singleton, object lifetime will be likely messed up if UserManager::Hide() is not called. BTW, I've removed the crash statement. Just said "We must hide the user manager before the test ends." FYI, see https://cs.chromium.org/search/?q=UserManager::Hide&sq=package:chromium&type=cs for the comments (if any) in other files. Here's the stack trace anyway. [7452:5488:0726/175008:FATAL:resource_dispatcher_host_impl.cc(1431)] Check failed: ContainsKey(active_resource_contexts_, resource_context). Backtrace: base::debug::StackTrace::StackTrace [0x68E0EA37+23] (f:\chromium\src\base\debug\stack_trace_win.cc:215) logging::LogMessage::~LogMessage [0x68E303B7+55] (f:\chromium\src\base\logging.cc:524) content::ResourceDispatcherHostImpl::BeginRequest [0x65DC6765+581] (f:\chromium\src\content\browser\loader\resource_dispatcher_host_impl.cc:1435) content::ResourceDispatcherHostImpl::OnRequestResource [0x65DCC3BC+204] (f:\chromium\src\content\browser\loader\resource_dispatcher_host_impl.cc:1242) IPC::MessageT<ResourceHostMsg_RequestResource_Meta,std::tuple<int,int,content::ResourceRequest>,void>::Dispatch<content::ResourceDispatcherHostImpl,content::ResourceDispatcherHostImpl,void,void (__thiscall content::ResourceDispatcherHostImpl::*)(int,int,c [0x65DBFA9E+286] (f:\chromium\src\ipc\ipc_message_templates.h:121) content::ResourceDispatcherHostImpl::OnMessageReceived [0x65DCBE8D+125] (f:\chromium\src\content\browser\loader\resource_dispatcher_host_impl.cc:1184) content::ResourceMessageFilter::OnMessageReceived [0x65DD5F13+19] (f:\chromium\src\content\browser\loader\resource_message_filter.cc:50) content::BrowserMessageFilter::Internal::OnMessageReceived [0x65A9863D+237] (f:\chromium\src\content\public\browser\browser_message_filter.cc:70) IPC::`anonymous namespace'::TryFiltersImpl [0x656B04BB+43] (f:\chromium\src\ipc\message_filter_router.cc:22) IPC::MessageFilterRouter::TryFilters [0x656B0474+68] (f:\chromium\src\ipc\message_filter_router.cc:87) IPC::ChannelProxy::Context::TryFilters [0x6569DDFD+29] (f:\chromium\src\ipc\ipc_channel_proxy.cc:80) IPC::ChannelProxy::Context::OnMessageReceived [0x6569D51E+14] (f:\chromium\src\ipc\ipc_channel_proxy.cc:97) IPC::ChannelMojo::OnMessageReceived [0x62FB99C6+278] (f:\chromium\src\ipc\mojo\ipc_channel_mojo.cc:405) IPC::internal::MessagePipeReader::Receive [0x62FBE3A2+306] (f:\chromium\src\ipc\mojo\ipc_message_pipe_reader.cc:139) IPC::mojom::ChannelStub::Accept [0x62FB538D+397] (f:\chromium\src\out\release\gen\ipc\mojo\ipc.mojom.cc:607) mojo::internal::InterfaceEndpointClient::HandleValidatedMessage [0x62FC8ECC+476] (f:\chromium\src\mojo\public\cpp\bindings\lib\interface_endpoint_client.cc:309) IPC::mojom::ChannelRequestValidator::Accept [0x62FB51D3+99] (f:\chromium\src\out\release\gen\ipc\mojo\ipc.mojom.cc:646) mojo::internal::InterfaceEndpointClient::HandleIncomingMessage [0x62FC8CD8+24] (f:\chromium\src\mojo\public\cpp\bindings\lib\interface_endpoint_client.cc:260) mojo::internal::MultiplexRouter::ProcessIncomingMessage [0x62FC3DA5+325] (f:\chromium\src\mojo\public\cpp\bindings\lib\multiplex_router.cc:776) mojo::internal::MultiplexRouter::Accept [0x62FC23CC+76] (f:\chromium\src\mojo\public\cpp\bindings\lib\multiplex_router.cc:494) mojo::internal::MessageHeaderValidator::Accept [0x62FCCF11+81] (f:\chromium\src\mojo\public\cpp\bindings\lib\message_header_validator.cc:73) mojo::internal::Connector::ReadAllAvailableMessages [0x62FCC8CA+154] (f:\chromium\src\mojo\public\cpp\bindings\lib\connector.cc:297) mojo::internal::Connector::OnWatcherHandleReady [0x62FCC6FE+46] (f:\chromium\src\mojo\public\cpp\bindings\lib\connector.cc:210) base::internal::Invoker<base::IndexSequence<0>,base::internal::BindState<base::internal::RunnableAdapter<void (__thiscall mojo::internal::Connector::*)(unsigned int)>,void __cdecl(mojo::internal::Connector *,unsigned int),base::internal::UnretainedWrapper [0x62FCCB93+19] (f:\chromium\src\base\bind_internal.h:368) mojo::Watcher::OnHandleReady [0x62FD1194+100] (f:\chromium\src\mojo\public\cpp\system\watcher.cc:122) mojo::Watcher::CallOnHandleReady [0x62FD0FAE+46] (f:\chromium\src\mojo\public\cpp\system\watcher.cc:142) mojo::edk::`anonymous namespace'::CallWatchCallback [0x64F2D2E7+23] (f:\chromium\src\mojo\edk\system\core.cc:58) base::internal::Invoker<base::IndexSequence<0,1>,base::internal::BindState<base::internal::RunnableAdapter<void (__cdecl*)(void (__cdecl*)(unsigned int,unsigned int,MojoHandleSignalsState,unsigned int),unsigned int,unsigned int,mojo::edk::HandleSignalsSta [0x64F306BE+30] (f:\chromium\src\base\bind_internal.h:364) mojo::edk::Watcher::MaybeInvokeCallback [0x64F4DDB5+53] (f:\chromium\src\mojo\edk\system\watcher.cc:24) mojo::edk::RequestContext::~RequestContext [0x64F485E1+145] (f:\chromium\src\mojo\edk\system\request_context.cc:50) mojo::edk::NodeChannel::OnChannelMessage [0x64F3B655+821] (f:\chromium\src\mojo\edk\system\node_channel.cc:707) mojo::edk::Channel::OnReadComplete [0x64F2A438+360] (f:\chromium\src\mojo\edk\system\channel.cc:554) mojo::edk::`anonymous namespace'::ChannelWin::OnReadDone [0x64F2B400+32] (f:\chromium\src\mojo\edk\system\channel_win.cc:236) mojo::edk::`anonymous namespace'::ChannelWin::OnIOCompleted [0x64F2B333+115] (f:\chromium\src\mojo\edk\system\channel_win.cc:225) base::MessagePumpForIO::WaitForWork [0x68E3FDF9+73] (f:\chromium\src\base\message_loop\message_pump_win.cc:674) base::MessagePumpForIO::DoRunLoop [0x68E3EFAE+174] (f:\chromium\src\base\message_loop\message_pump_win.cc:636) base::MessagePumpWin::Run [0x68E3F9CA+74] (f:\chromium\src\base\message_loop\message_pump_win.cc:60) base::MessageLoop::RunHandler [0x68E3CBC1+17] (f:\chromium\src\base\message_loop\message_loop.cc:439) base::MessageLoop::Run [0x68E3CB8D+29] (f:\chromium\src\base\message_loop\message_loop.cc:295) base::Thread::Run [0x68E957AB+11] (f:\chromium\src\base\threading\thread.cc:205) content::BrowserThreadImpl::IOThreadRun [0x65BAEDF0+32] (f:\chromium\src\content\browser\browser_thread_impl.cc:216) content::BrowserThreadImpl::Run [0x65BAF85A+362] (f:\chromium\src\content\browser\browser_thread_impl.cc:251) base::Thread::ThreadMain [0x68E95D6F+319] (f:\chromium\src\base\threading\thread.cc:259) base::`anonymous namespace'::ThreadFunc [0x68E8CF82+130] (f:\chromium\src\base\threading\platform_thread_win.cc:86) BaseThreadInitThunk [0x750538F4+36] RtlUnicodeStringToInteger [0x77895DE3+595] RtlUnicodeStringToInteger [0x77895DAE+542] https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:98: void WaitForAndHideUserManager() { On 2016/07/19 21:56:08, Peter Kasting wrote: > Nit: Maybe name this ExpectUserManagerShown() since from the caller perspective > that's the critical bit (hiding the user manager afterwards is more like an > implementation detail). SGTM. I've renamed to ExpectUserManagerToShow. (It may be already shown, or will show in the future.) https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:140: // importantly remove directories before the browser process starts. On 2016/07/19 21:56:07, Peter Kasting wrote: > How come we can't do the steps in these functions inside the PRE_ functions > instead, and collapse each test from three parts to two? When the browser process in running we cannot remove directories. That's why we need to delete the directories in SetUpUserDataDirectory(), which is called before the browser process starts. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:221: // Most of the tests below has three sections: On 2016/07/19 21:56:08, Peter Kasting wrote: > Nit: has -> have > > Somewhere in this comment you should probably also explain why this > functionality can't all be in one function for each test (like is commonly > done). Updated the comment so it carries more details. I did not directly tell why it is not feasible to do it in one function though. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:225: // (3) The main test, where the real test is taken. On 2016/07/19 21:56:07, Peter Kasting wrote: > Nit: Just "The test itself." is probably fine Done. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:248: // missing, it should fallback to any last opened profiles. On 2016/07/19 21:56:08, Peter Kasting wrote: > Nit: When using "fallback" as a verb, spell it "fall back". "fallback" is a > noun, or adjective. (Several places) Done. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:266: // LastUsedProfileFallbackToUserManager : If all last used profile is On 2016/07/19 21:56:07, Peter Kasting wrote: > Nit: profile is -> profiles are Done. Anyway, I should have referred to "last opened profiles" instead of "last used profile". https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:267: // missing, it should fallback to user manager. To open the user manager, both On 2016/07/19 21:56:07, Peter Kasting wrote: > Nit: it should fallback to -> fall back to the Done. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:268: // the guesst profile and the system profile must be creatable. On 2016/07/19 21:56:08, Peter Kasting wrote: > Nit: guest Done. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:301: // Create the guest profile path only. The system profile cannot be created. On 2016/07/19 21:56:08, Peter Kasting wrote: > Nit: "Create the guest profile path, but not the system profile one. This will > make it impossible to create the system profile once the permissions are locked > down during setup." Done. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:318: // guest profile cannot be opened, fallback to any non-signin profile. On 2016/07/19 21:56:08, Peter Kasting wrote: > Nit: non-signin -> not-signed-in Oh. Per Mike's comment before, I'd change this to "any profile that is not locked". https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:339: // an appropriate exit code (0). On 2016/07/19 21:56:07, Peter Kasting wrote: > Nit: This doesn't check the browser exit code, so I wouldn't talk about it. The browser exit code is checked somewhere else in InProcessBrowserTest. For InProcessBrowserTest, somehow returning a zero means the test passed even if the test body is not run. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:353: // fallback to in this test, chromium should not start. On 2016/07/19 21:56:07, Peter Kasting wrote: > Nit: "Profiles that are locked should never be initialized. Since there are no > unlocked profiles, the browser should not start." Done. Anyway the test is named incorrectly, so I renamed to DoNotStartLockedProfile. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:356: // Sign out the default profile and wait for the user manager. On 2016/07/19 21:56:07, Peter Kasting wrote: > Why is the user manager appearing during the PRE_ test? Isn't this just setup > before the browser starts? When a profile is being locked the user manager shows, allowing the user to use the browser with the same or another profile. This is just some implementation detail, which has nothing to do with the idea of the test. Anyway, added a short explanation in the comment. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:373: // profile is selected by --profile-directory switch. Chromium should not start On 2016/07/19 21:56:07, Peter Kasting wrote: > Nit: by -> by the; Chromium -> The browser Done. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:374: // if the specified profile could not initialize. On 2016/07/19 21:56:07, Peter Kasting wrote: > Nit: initialize -> be initialized Done. https://codereview.chromium.org/2047483003/diff/300001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:397: // attempt to open the profile. Instead shows the user manager. On 2016/07/19 21:56:07, Peter Kasting wrote: > Nit: "If the profile's entry in ProfileAttributesStorage is missing, the profile > is deleted. The browser should not attempt to open the profile, but should show > the user manager instead." Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by lwchkg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by lwchkg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2047483003/diff/360001/base/test/test_file_ut... File base/test/test_file_util.h (right): https://codereview.chromium.org/2047483003/diff/360001/base/test/test_file_ut... base/test/test_file_util.h:48: // ACCESS_MARK structure, and its possible values can be found by searching MARK -> MASK https://codereview.chromium.org/2047483003/diff/360001/base/test/test_file_ut... base/test/test_file_util.h:49: // "file and directory access rights constants". Don't say this; "search" implies codesearch. Link directly to MSDN. We do this many other places. If you want to directly talk about ACCESS_MASKs, use https://msdn.microsoft.com/en-us/library/windows/desktop/aa374892(v=vs.85).aspx . https://codereview.chromium.org/2047483003/diff/360001/base/test/test_file_ut... File base/test/test_file_util_win.cc (right): https://codereview.chromium.org/2047483003/diff/360001/base/test/test_file_ut... base/test/test_file_util_win.cc:80: std::unique_ptr<T[]> ToCStr(const std::basic_string<T>& str) { Don't templatize this. Use "std::wstring" directly. The templatization is a lie anyway; wcsncpy() requires that its arguments be wchar_t*, so |str| has to be a wstring. And it always is, since this file is only built on Windows, and FilePath::value() returns a wstring there. https://codereview.chromium.org/2047483003/diff/360001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2047483003/diff/360001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:931: for (Profile* profile : last_opened_profiles) { Nit: You didn't actually inline |last_opened_profiles| into this statement. https://codereview.chromium.org/2047483003/diff/360001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc (right): https://codereview.chromium.org/2047483003/diff/360001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:135: // importantly remove directories before the browser process starts. How about just: For each test, declare a SetUpUserDataDirectory[testname] function. The comment below this class explains what these are for. https://codereview.chromium.org/2047483003/diff/360001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:221: // and to prevent the profile directories from being created again in (3). Nit: Maybe add "We can't remove these directories while the browser process is running, so that can't be done during (1) or (3)." https://codereview.chromium.org/2047483003/diff/360001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:222: // (3) The main test, where the test itself is taken. With the Nit: I still think the first sentence here would be better as just "The test itself." https://codereview.chromium.org/2047483003/diff/360001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:342: // an appropriate exit code (0). Nit: Maybe "If no startup option is feasible, the browser should quit cleanly." (I'm still trying to avoid specifically talking about exit codes.)
The CQ bit was checked by lwchkg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Landing. Thanks everyone! https://codereview.chromium.org/2047483003/diff/360001/base/test/test_file_ut... File base/test/test_file_util.h (right): https://codereview.chromium.org/2047483003/diff/360001/base/test/test_file_ut... base/test/test_file_util.h:48: // ACCESS_MARK structure, and its possible values can be found by searching On 2016/07/28 20:09:49, Peter Kasting wrote: > MARK -> MASK Done, and thanks for catching. https://codereview.chromium.org/2047483003/diff/360001/base/test/test_file_ut... base/test/test_file_util.h:49: // "file and directory access rights constants". On 2016/07/28 20:09:49, Peter Kasting wrote: > Don't say this; "search" implies codesearch. > > Link directly to MSDN. We do this many other places. If you want to directly > talk about ACCESS_MASKs, use > https://msdn.microsoft.com/en-us/library/windows/desktop/aa374892(v=vs.85).aspx > . Thanks for explanation. I've also included https://msdn.microsoft.com/en-us/library/aa822867.aspx because it includes the hard-to-find possible values. https://codereview.chromium.org/2047483003/diff/360001/base/test/test_file_ut... File base/test/test_file_util_win.cc (right): https://codereview.chromium.org/2047483003/diff/360001/base/test/test_file_ut... base/test/test_file_util_win.cc:80: std::unique_ptr<T[]> ToCStr(const std::basic_string<T>& str) { On 2016/07/28 20:09:49, Peter Kasting wrote: > Don't templatize this. Use "std::wstring" directly. > > The templatization is a lie anyway; wcsncpy() requires that its arguments be > wchar_t*, so |str| has to be a wstring. And it always is, since this file is > only built on Windows, and FilePath::value() returns a wstring there. Done. https://codereview.chromium.org/2047483003/diff/360001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2047483003/diff/360001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:931: for (Profile* profile : last_opened_profiles) { On 2016/07/28 20:09:49, Peter Kasting wrote: > Nit: You didn't actually inline |last_opened_profiles| into this statement. Oops. Looks like I misunderstood your last comment. Done finally. https://codereview.chromium.org/2047483003/diff/360001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc (right): https://codereview.chromium.org/2047483003/diff/360001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:135: // importantly remove directories before the browser process starts. On 2016/07/28 20:09:50, Peter Kasting wrote: > How about just: > > For each test, declare a SetUpUserDataDirectory[testname] function. The comment > below this class explains what these are for. Sounds good and done. We don't need to tell twice. https://codereview.chromium.org/2047483003/diff/360001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:221: // and to prevent the profile directories from being created again in (3). On 2016/07/28 20:09:50, Peter Kasting wrote: > Nit: Maybe add "We can't remove these directories while the browser process is > running, so that can't be done during (1) or (3)." Done. https://codereview.chromium.org/2047483003/diff/360001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:222: // (3) The main test, where the test itself is taken. With the On 2016/07/28 20:09:50, Peter Kasting wrote: > Nit: I still think the first sentence here would be better as just "The test > itself." I see. Done. https://codereview.chromium.org/2047483003/diff/360001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc:342: // an appropriate exit code (0). On 2016/07/28 20:09:50, Peter Kasting wrote: > Nit: Maybe "If no startup option is feasible, the browser should quit cleanly." > > (I'm still trying to avoid specifically talking about exit codes.) Okay, and done.
The CQ bit was checked by lwchkg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlerman@chromium.org, brettw@chromium.org, phajdan.jr@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2047483003/#ps440001 (title: "Fix a mistake in a comment. (Didn't check Atom reflow output, sorry :-( )")
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.
Description was changed from ========== Add fallback behavior if the last used profile cannot initialize Before this CL, if the last used profile cannot be initialized, it will be a silent exit in Windows and a CHECK failure in other platforms. In this CL a list of possible fall-back behavior are used (from the top to the bottom): (1) open other last opened profiles which was not signed out. (2) open the user manager (guest profile & system profile) (3) open other profiles which was not signed out. (4) at last, show a message box and do not start chromium. BUG=614753 ========== to ========== Add fallback behavior if the last used profile cannot initialize Before this CL, if the last used profile cannot be initialized, it will be a silent exit in Windows and a CHECK failure in other platforms. In this CL a list of possible fall-back behavior are used (from the top to the bottom): (1) open other last opened profiles which was not signed out. (2) open the user manager (guest profile & system profile) (3) open other profiles which was not signed out. (4) at last, show a message box and do not start chromium. BUG=614753 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== Add fallback behavior if the last used profile cannot initialize Before this CL, if the last used profile cannot be initialized, it will be a silent exit in Windows and a CHECK failure in other platforms. In this CL a list of possible fall-back behavior are used (from the top to the bottom): (1) open other last opened profiles which was not signed out. (2) open the user manager (guest profile & system profile) (3) open other profiles which was not signed out. (4) at last, show a message box and do not start chromium. BUG=614753 ========== to ========== Add fallback behavior if the last used profile cannot initialize Before this CL, if the last used profile cannot be initialized, it will be a silent exit in Windows and a CHECK failure in other platforms. In this CL a list of possible fall-back behavior are used (from the top to the bottom): (1) open other last opened profiles which was not signed out. (2) open the user manager (guest profile & system profile) (3) open other profiles which was not signed out. (4) at last, show a message box and do not start chromium. BUG=614753 Committed: https://crrev.com/2873496dd5bb12be052aa1c5a341637faade926d Cr-Commit-Position: refs/heads/master@{#409023} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/2873496dd5bb12be052aa1c5a341637faade926d Cr-Commit-Position: refs/heads/master@{#409023} |
