Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1782443006:
Create LoadProfile method in profile_manager (Closed)
|
Created:
4 years, 9 months ago by Miguel Garcia Modified:
4 years, 9 months ago CC:
chromium-reviews, rginda+watch_chromium.org, Bernhard Bauer Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate a LoadProfile method in profile_manager
It loads a profile given its name and whether to use the incognito
version.
BUG=571056
Committed: https://crrev.com/1b495549523fcccb2290c2d3b9f10d54794557b6
Cr-Commit-Position: refs/heads/master@{#382013}
Patch Set 1 : Mac implementation ported. Android not yet #Patch Set 2 : #
Total comments: 16
Patch Set 3 : #
Total comments: 17
Patch Set 4 : #Patch Set 5 : #
Messages
Total messages: 24 (10 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Create LoadProfile method in profile_manager It loads a profile given its name and wether to use the incognito version. BUG=571056 ========== to ========== Create a LoadProfile method in profile_manager It loads a profile given its name and whether to use the incognito version. BUG=571056 ==========
miguelg@chromium.org changed reviewers: + peter@chromium.org
Peter, do you mind having a look in Bernhard's abscence before I add OWNERS for /profile ? I will also need your LGTM for platform_notification_service_impl.cc I have not ported android yet but I'll do it before moving forward if you agree with the overall direction. Can also do it in a different CL but this seems relatively small.
Turns the android bot failure was just a flake so this should be ready to go. Tested on bot mac and android. On 2016/03/10 17:31:13, Miguel Garcia wrote: > Peter, do you mind having a look in Bernhard's abscence before I add OWNERS for > /profile ? I will also need your LGTM for platform_notification_service_impl.cc > > I have not ported android yet but I'll do it before moving forward if you agree > with the overall direction. Can also do it in a different CL but this seems > relatively small.
notifications lgtm % suggestion. The rest looks fine to me modulo a few nits. https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/notifica... chrome/browser/notifications/platform_notification_service_impl.cc:105: if (!profile) We should probably log something if the profile could not be loaded? Maybe we should have define a new content::PersistentNotificationStatus for this (PERSISTENT_NOTIFICATION_STATUS_INVALID_PROFILE?) https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:266: // so ignore it. nit: avoid "we". // This is an intermediate state where the profile has been created, but is // not yet initialized. Ignore this and wait for the next state change. https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:266: // so ignore it. nit: the description for this function does not make it obvious that it can be called multiple times with different |status| values. https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:443: base::string16(), std::string(), std::string()); nit: this would benefit from inline comments about the parameter names. CreateProfileAsync(profile_path, base::Bind(&OnProfileLoaded, callback, incognito), base::string16() /* name */, std::string() /* icon_url */, std::string() /* supervided_user_id */); https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.h:95: // Should be called on the UI thread. This should document |incognito| as well, have proper sentences and a few editorial nits. What about: // Asynchronously loads an existing profile given its |profile_name| within // the user data directory, optionally in |incognito| mode. The |callback| // will be called with the Profile when it has been loaded, or with a nullptr // otherwise. Should be called on the UI thread. https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.h:95: // Should be called on the UI thread. Should we match GetProfile() above and swap out the |profile_name| std::string with a base::FilePath called |profile_dir|? This would make it easy to have the same fast-path (GetProfileByPath()) as in that method. https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager_unittest.cc (right): https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager_unittest.cc:104: EXPECT_EQ(profile->IsOffTheRecord(), incognito); nit: for lines 104, 107 and 110, change the parameter ordering: EXPECT_*(expected, actual); (This may soon become redundant as the latest GTest treats both parameters in the same way, but Chrome hasn't updated yet.)
miguelg@chromium.org changed reviewers: + anthonyvd@chromium.org
PTAL Adding anthonyvd to review profile_manager* Thanks! https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/notifica... chrome/browser/notifications/platform_notification_service_impl.cc:105: if (!profile) On 2016/03/11 17:45:15, Peter Beverloo wrote: > We should probably log something if the profile could not be loaded? Maybe we > should have define a new content::PersistentNotificationStatus for this > (PERSISTENT_NOTIFICATION_STATUS_INVALID_PROFILE?) Will add a TODO to add the state and UMA to it and add logging for now. https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:266: // so ignore it. On 2016/03/11 17:45:15, Peter Beverloo wrote: > nit: avoid "we". > > // This is an intermediate state where the profile has been created, but is > // not yet initialized. Ignore this and wait for the next state change. Done. https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:266: // so ignore it. On 2016/03/11 17:45:15, Peter Beverloo wrote: > nit: the description for this function does not make it obvious that it can be > called multiple times with different |status| values. Done. https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:443: base::string16(), std::string(), std::string()); On 2016/03/11 17:45:15, Peter Beverloo wrote: > nit: this would benefit from inline comments about the parameter names. > > CreateProfileAsync(profile_path, > base::Bind(&OnProfileLoaded, callback, incognito), > base::string16() /* name */, std::string() /* icon_url */, > std::string() /* supervided_user_id */); Done. https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.h:95: // Should be called on the UI thread. On 2016/03/11 17:45:15, Peter Beverloo wrote: > This should document |incognito| as well, have proper sentences and a few > editorial nits. What about: > > // Asynchronously loads an existing profile given its |profile_name| within > // the user data directory, optionally in |incognito| mode. The |callback| > // will be called with the Profile when it has been loaded, or with a nullptr > // otherwise. Should be called on the UI thread. Done. https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.h:95: // Should be called on the UI thread. On 2016/03/11 17:45:15, Peter Beverloo wrote: > Should we match GetProfile() above and swap out the |profile_name| std::string > with a base::FilePath called |profile_dir|? This would make it easy to have the > same fast-path (GetProfileByPath()) as in that method. Let's let the owners decide. I am fine swapping it. The difference though is that in the other methods that is actually the full FilePath whereas here we want to leave the fact that we are passing the last component of the path as an implementation detail. https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager_unittest.cc (right): https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager_unittest.cc:104: EXPECT_EQ(profile->IsOffTheRecord(), incognito); On 2016/03/11 17:45:16, Peter Beverloo wrote: > nit: for lines 104, 107 and 110, change the parameter ordering: > > EXPECT_*(expected, actual); > > (This may soon become redundant as the latest GTest treats both parameters in > the same way, but Chrome hasn't updated yet.) Done.
bauerb@chromium.org changed reviewers: + bauerb@chromium.org
https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager_unittest.cc (right): https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager_unittest.cc:104: EXPECT_EQ(profile->IsOffTheRecord(), incognito); On 2016/03/11 17:45:16, Peter Beverloo wrote: > nit: for lines 104, 107 and 110, change the parameter ordering: > > EXPECT_*(expected, actual); > > (This may soon become redundant as the latest GTest treats both parameters in > the same way, but Chrome hasn't updated yet.) For a given value of "soon", because we can't roll GTest, as it has been merged with GMock, in a directory structure that is different from what we use (see crbug.com/539853#c11). https://codereview.chromium.org/1782443006/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager_unittest.cc:330: MockObserver mock_observer1; Nit: no "1" suffix? https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:256: // It will then run |client_callback| with the right profile or NULL if it was Nit: Now that NULL is deprecated, use "null" for a conceptual null value (along with |nullptr| for the constant) :) https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:278: } Nit: empty line after this one. https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:440: LOG(ERROR) << "Loading a profile path that does not exist"; This would be a programmer error (i.e. a bug), right? In that case DCHECK would be more appropriate. https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.h:94: // otherwise. Should be called on the UI thread. Can you point out what the difference to CreateProfileAsync is? (Namely, that this will never create a new profile if one doesn't already exist on disk, right?) https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager_unittest.cc (right): https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager_unittest.cc:96: void ExpectNullProfile(Profile* profile) { Can you make this take an additional Closure to run and then pass in a QuitClosure from the RunLoop? That way you actually know that this method is called (and I don't really like relying on RunUntilIdle). https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager_unittest.cc:105: if (!incognito) { For incognito profiles, you could verify the name of the original profile. https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager_unittest.cc:107: EXPECT_EQ(base::ASCIIToUTF16(profile_name), Alternatively, use FilePath().AppendASCII(profile_name)? Having to create an empty FilePath isn't super elegant either, but it avoids the platform #ifdef (which is even more ick IMO). (Really, FilePath should have a FromASCII() method.) https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager_unittest.cc:315: TEST_F(ProfileManagerTest, LoadProfileDoesNotExist) { Nit: "LoadNonExistingProfile" (and "LoadExistingProfile" below)?
Thanks for the review! PTAL anthonyvd@ could you have a look as well for OWNERS approval? https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:256: // It will then run |client_callback| with the right profile or NULL if it was On 2016/03/14 19:05:34, Bernhard Bauer wrote: > Nit: Now that NULL is deprecated, use "null" for a conceptual null value (along > with |nullptr| for the constant) :) Done. https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:278: } On 2016/03/14 19:05:34, Bernhard Bauer wrote: > Nit: empty line after this one. Done. https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:440: LOG(ERROR) << "Loading a profile path that does not exist"; On 2016/03/14 19:05:34, Bernhard Bauer wrote: > This would be a programmer error (i.e. a bug), right? In that case DCHECK would > be more appropriate. I believe this can still happen if there is a problem loading the profile from disk. https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.h (right): https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.h:94: // otherwise. Should be called on the UI thread. On 2016/03/14 19:05:34, Bernhard Bauer wrote: > Can you point out what the difference to CreateProfileAsync is? (Namely, that > this will never create a new profile if one doesn't already exist on disk, > right?) Done. https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager_unittest.cc (right): https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager_unittest.cc:96: void ExpectNullProfile(Profile* profile) { On 2016/03/14 19:05:34, Bernhard Bauer wrote: > Can you make this take an additional Closure to run and then pass in a > QuitClosure from the RunLoop? That way you actually know that this method is > called (and I don't really like relying on RunUntilIdle). I will look into this and fix it before submitting. Feel free to LGTM pending this change. Just wanted to make sure it can go through a second round of comments today. https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager_unittest.cc:105: if (!incognito) { On 2016/03/14 19:05:34, Bernhard Bauer wrote: > For incognito profiles, you could verify the name of the original profile. Good idea! https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager_unittest.cc:107: EXPECT_EQ(base::ASCIIToUTF16(profile_name), On 2016/03/14 19:05:34, Bernhard Bauer wrote: > Alternatively, use FilePath().AppendASCII(profile_name)? Having to create an > empty FilePath isn't super elegant either, but it avoids the platform #ifdef > (which is even more ick IMO). > > (Really, FilePath should have a FromASCII() method.) Agreed. Done https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager_unittest.cc:315: TEST_F(ProfileManagerTest, LoadProfileDoesNotExist) { On 2016/03/14 19:05:34, Bernhard Bauer wrote: > Nit: "LoadNonExistingProfile" (and "LoadExistingProfile" below)? Done.
lgtm
https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1782443006/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:440: LOG(ERROR) << "Loading a profile path that does not exist"; On 2016/03/17 20:04:07, Miguel Garcia wrote: > On 2016/03/14 19:05:34, Bernhard Bauer wrote: > > This would be a programmer error (i.e. a bug), right? In that case DCHECK > would > > be more appropriate. > > I believe this can still happen if there is a problem loading the profile from > disk. No, that's the status != Profile::CREATE_STATUS_INITIALIZED case above, where you are already logging a warning. This here means you are trying to load a profile that Chrome doesn't know about.
Bernhard, added a boolean for the LoadProfile return type as discussed offline. Also made the change in the test as requested. PTAL
On 2016/03/18 14:24:24, Miguel Garcia wrote: > Bernhard, added a boolean for the LoadProfile return type as discussed offline. > > Also made the change in the test as requested. > > PTAL The win8 bot seems like a flake, so if you don't mind and if you think this is l-g-t-m could you also click the commit check box?
The CQ bit was checked by bauerb@chromium.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, anthonyvd@chromium.org Link to the patchset: https://codereview.chromium.org/1782443006/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1782443006/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1782443006/120001
Message was sent while issue was closed.
Description was changed from ========== Create a LoadProfile method in profile_manager It loads a profile given its name and whether to use the incognito version. BUG=571056 ========== to ========== Create a LoadProfile method in profile_manager It loads a profile given its name and whether to use the incognito version. BUG=571056 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Create a LoadProfile method in profile_manager It loads a profile given its name and whether to use the incognito version. BUG=571056 ========== to ========== Create a LoadProfile method in profile_manager It loads a profile given its name and whether to use the incognito version. BUG=571056 Committed: https://crrev.com/1b495549523fcccb2290c2d3b9f10d54794557b6 Cr-Commit-Position: refs/heads/master@{#382013} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1b495549523fcccb2290c2d3b9f10d54794557b6 Cr-Commit-Position: refs/heads/master@{#382013} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
