Move ProfileLoader to chrome/browser/profiles.

Move ProfileLoader to chrome/browser/profiles.

The class fits better here because it only deals with profiles.
Added profile_loader_unittest.cc

BUG=168080
TEST=There should be no change in behavior.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207745

[tapted, 2013-06-11 05:31:44]

+chrome-apps-syd-reviews

app_lists's profile_loader.h relies on some guarantee that the profile path
actually exists, before trying to load it. e.g. AppListServiceImpl does this by
listening for OnProfileWillBeRemoved, but I think shims are too decoupled from
the profile to do the same.

I'm currently leaning towards a better entrypoint into ProfileManager, e.g.
'LoadProfileAsync' rather than 'CreateProfileAsync'. ProfileManager is already a
bit busy, so I hesitate to suggest this, but I'm not sure there's a smart way to
do it outside of that.

https://codereview.chromium.org/16766003/diff/1/apps/app_shim/app_shim_host_m...
File apps/app_shim/app_shim_host_mac.cc (right):

https://codereview.chromium.org/16766003/diff/1/apps/app_shim/app_shim_host_m...
apps/app_shim/app_shim_host_mac.cc:109: if (!profile) {
I don't think there's a way for a NULL pointer to ever pass through the
callback, the way profile_loader.cc is currently implemented. But, maybe it
needs some asynchronous error handling / error state or 'OnError' callback.

E.g. What happens if the profile has been deleted? profile_loader.cc uses
CreateProfileAsync which I think will try to recreate it, which would be bad.

https://codereview.chromium.org/16766003/diff/1/apps/app_shim/app_shim_host_m...
File apps/app_shim/app_shim_host_mac.h (right):

https://codereview.chromium.org/16766003/diff/1/apps/app_shim/app_shim_host_m...
apps/app_shim/app_shim_host_mac.h:14: #include
"chrome/browser/ui/app_list/profile_loader.h"
Hmm. This makes something of a circular dependency. I think we need to move
profile_loader to live under /apps, or at least higher up than app_list. It
doesn't make sense for the shim host to depend on what's currently the
app_list's profile loader. profile_loader.cc has includes:

#include "chrome/browser/lifetime/application_lifetime.h"
#include "chrome/browser/profiles/profile_manager.h"

profile_manager.h is already looped in to apps/DEPS, but I think you'll get a
presubmit/upload warning for application_lifetime.h.

So then the question becomes whether chrome::{Start,End}KeepAlive is something
that things in /apps should know about, or whether it should stay under
chrome/browser.

[jackhou1, 2013-06-12 07:47:33]

On 2013/06/11 05:31:44, tapted wrote:
> +chrome-apps-syd-reviews
> 
> app_lists's profile_loader.h relies on some guarantee that the profile path
> actually exists, before trying to load it. e.g. AppListServiceImpl does this
by
> listening for OnProfileWillBeRemoved, but I think shims are too decoupled from
> the profile to do the same.
> 
> I'm currently leaning towards a better entrypoint into ProfileManager, e.g.
> 'LoadProfileAsync' rather than 'CreateProfileAsync'. ProfileManager is already
a
> bit busy, so I hesitate to suggest this, but I'm not sure there's a smart way
to
> do it outside of that.
> 
>
https://codereview.chromium.org/16766003/diff/1/apps/app_shim/app_shim_host_m...
> File apps/app_shim/app_shim_host_mac.cc (right):
> 
>
https://codereview.chromium.org/16766003/diff/1/apps/app_shim/app_shim_host_m...
> apps/app_shim/app_shim_host_mac.cc:109: if (!profile) {
> I don't think there's a way for a NULL pointer to ever pass through the
> callback, the way profile_loader.cc is currently implemented. But, maybe it
> needs some asynchronous error handling / error state or 'OnError' callback.
> 
> E.g. What happens if the profile has been deleted? profile_loader.cc uses
> CreateProfileAsync which I think will try to recreate it, which would be bad.
> 
>
https://codereview.chromium.org/16766003/diff/1/apps/app_shim/app_shim_host_m...
> File apps/app_shim/app_shim_host_mac.h (right):
> 
>
https://codereview.chromium.org/16766003/diff/1/apps/app_shim/app_shim_host_m...
> apps/app_shim/app_shim_host_mac.h:14: #include
> "chrome/browser/ui/app_list/profile_loader.h"
> Hmm. This makes something of a circular dependency. I think we need to move
> profile_loader to live under /apps, or at least higher up than app_list. It
> doesn't make sense for the shim host to depend on what's currently the
> app_list's profile loader. profile_loader.cc has includes:
> 
> #include "chrome/browser/lifetime/application_lifetime.h"
> #include "chrome/browser/profiles/profile_manager.h"
> 
> profile_manager.h is already looped in to apps/DEPS, but I think you'll get a
> presubmit/upload warning for application_lifetime.h.
> 
> So then the question becomes whether chrome::{Start,End}KeepAlive is something
> that things in /apps should know about, or whether it should stay under
> chrome/browser.

benwells, what do you think?

[benwells, 2013-06-12 08:14:19]

On 2013/06/12 07:47:33, jackhou1 wrote:
> On 2013/06/11 05:31:44, tapted wrote:
> > +chrome-apps-syd-reviews
> > 
> > app_lists's profile_loader.h relies on some guarantee that the profile path
> > actually exists, before trying to load it. e.g. AppListServiceImpl does this
> by
> > listening for OnProfileWillBeRemoved, but I think shims are too decoupled
from
> > the profile to do the same.
> > 
> > I'm currently leaning towards a better entrypoint into ProfileManager, e.g.
> > 'LoadProfileAsync' rather than 'CreateProfileAsync'. ProfileManager is
already
> a
> > bit busy, so I hesitate to suggest this, but I'm not sure there's a smart
way
> to
> > do it outside of that.
> > 
> >
>
https://codereview.chromium.org/16766003/diff/1/apps/app_shim/app_shim_host_m...
> > File apps/app_shim/app_shim_host_mac.cc (right):
> > 
> >
>
https://codereview.chromium.org/16766003/diff/1/apps/app_shim/app_shim_host_m...
> > apps/app_shim/app_shim_host_mac.cc:109: if (!profile) {
> > I don't think there's a way for a NULL pointer to ever pass through the
> > callback, the way profile_loader.cc is currently implemented. But, maybe it
> > needs some asynchronous error handling / error state or 'OnError' callback.
> > 
> > E.g. What happens if the profile has been deleted? profile_loader.cc uses
> > CreateProfileAsync which I think will try to recreate it, which would be
bad.
> > 
> >
>
https://codereview.chromium.org/16766003/diff/1/apps/app_shim/app_shim_host_m...
> > File apps/app_shim/app_shim_host_mac.h (right):
> > 
> >
>
https://codereview.chromium.org/16766003/diff/1/apps/app_shim/app_shim_host_m...
> > apps/app_shim/app_shim_host_mac.h:14: #include
> > "chrome/browser/ui/app_list/profile_loader.h"
> > Hmm. This makes something of a circular dependency. I think we need to move
> > profile_loader to live under /apps, or at least higher up than app_list. It
> > doesn't make sense for the shim host to depend on what's currently the
> > app_list's profile loader. profile_loader.cc has includes:
> > 
> > #include "chrome/browser/lifetime/application_lifetime.h"
> > #include "chrome/browser/profiles/profile_manager.h"
> > 
> > profile_manager.h is already looped in to apps/DEPS, but I think you'll get
a
> > presubmit/upload warning for application_lifetime.h.
> > 
> > So then the question becomes whether chrome::{Start,End}KeepAlive is
something
> > that things in /apps should know about, or whether it should stay under
> > chrome/browser.
> 
> benwells, what do you think?

I am about to add a temporary dependency from apps to
c/b/lifetime/application_lifetime.h. Temporary as that code needs to come down
to apps in some way (in some way = with some refactoring).

So from a DEPS perspective I don't mind.

[jackhou1, 2013-06-12 09:41:32]

This now uses ProfileInfoCache to check if a profile_dir is valid before trying
to load the profile.

https://codereview.chromium.org/16766003/diff/1/apps/app_shim/app_shim_host_m...
File apps/app_shim/app_shim_host_mac.cc (right):

https://codereview.chromium.org/16766003/diff/1/apps/app_shim/app_shim_host_m...
apps/app_shim/app_shim_host_mac.cc:109: if (!profile) {
On 2013/06/11 05:31:44, tapted wrote:
> I don't think there's a way for a NULL pointer to ever pass through the
> callback, the way profile_loader.cc is currently implemented. But, maybe it
> needs some asynchronous error handling / error state or 'OnError' callback.
> 
> E.g. What happens if the profile has been deleted? profile_loader.cc uses
> CreateProfileAsync which I think will try to recreate it, which would be bad.

LoadProfile now runs this callback with NULL if the profile does not exist.

https://codereview.chromium.org/16766003/diff/1/apps/app_shim/app_shim_host_m...
File apps/app_shim/app_shim_host_mac.h (right):

https://codereview.chromium.org/16766003/diff/1/apps/app_shim/app_shim_host_m...
apps/app_shim/app_shim_host_mac.h:14: #include
"chrome/browser/ui/app_list/profile_loader.h"
On 2013/06/11 05:31:44, tapted wrote:
> Hmm. This makes something of a circular dependency. I think we need to move
> profile_loader to live under /apps, or at least higher up than app_list. It
> doesn't make sense for the shim host to depend on what's currently the
> app_list's profile loader. profile_loader.cc has includes:
> 
> #include "chrome/browser/lifetime/application_lifetime.h"
> #include "chrome/browser/profiles/profile_manager.h"
> 
> profile_manager.h is already looped in to apps/DEPS, but I think you'll get a
> presubmit/upload warning for application_lifetime.h.
> 
> So then the question becomes whether chrome::{Start,End}KeepAlive is something
> that things in /apps should know about, or whether it should stay under
> chrome/browser.

Done.

[jackhou1, 2013-06-13 03:20:49]

sail, could you please review for OWNERS in chrome/browser/profile/

[jackhou1, 2013-06-13 03:54:33]

On 2013/06/13 03:20:49, jackhou1 wrote:
> sail, could you please review for OWNERS in chrome/browser/profile/

I should add some context. After discussing with benwells and tapted, we think
ProfileLoader fits better in chrome/browser/profiles since it only deals with
profiles. Putting it there allows app_shim_host and app_list_service_impl to
neatly depend on it, and is an obvious place to look for anyone wanting to load
profiles asynchronously.

[sail, 2013-06-13 15:10:16]

Is it possible to test ProfileLoader in a unit_test with a testing profile?

https://codereview.chromium.org/16766003/diff/21001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_loader.cc (right):

https://codereview.chromium.org/16766003/diff/21001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_loader.cc:28: profile_load_sequence_id_++;
Use prefix? ++profile_load_sequence_id_

https://codereview.chromium.org/16766003/diff/21001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_loader.cc:45: profile_load_sequence_id_,
Shouldn't this function increment profile_load_sequence_id_? If it doesn't then
how would an existing profile load get invalidated?

https://codereview.chromium.org/16766003/diff/21001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_loader.h (right):

https://codereview.chromium.org/16766003/diff/21001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_loader.h:18: class ProfileLoader {
Needs class comments describing why someone might want to use this.

https://codereview.chromium.org/16766003/diff/21001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_loader.h:23: bool AnyProfilesLoading() const;
How about IsAnyProfileLoading() ?

[jackhou1, 2013-06-14 09:13:28]

I've removed the AppShimHost changes, so this CL is just for moving
profile_loader.

Added a unit test that mocks out the ProfileManager methods used by
ProfileLoader. I couldn't mock out ProfileManager because those methods are not
virtual.

https://codereview.chromium.org/16766003/diff/21001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_loader.cc (right):

https://codereview.chromium.org/16766003/diff/21001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_loader.cc:28: profile_load_sequence_id_++;
On 2013/06/13 15:10:16, sail wrote:
> Use prefix? ++profile_load_sequence_id_

Done.

https://codereview.chromium.org/16766003/diff/21001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_loader.cc:45: profile_load_sequence_id_,
On 2013/06/13 15:10:16, sail wrote:
> Shouldn't this function increment profile_load_sequence_id_? If it doesn't
then
> how would an existing profile load get invalidated?

Done.

https://codereview.chromium.org/16766003/diff/21001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_loader.h (right):

https://codereview.chromium.org/16766003/diff/21001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_loader.h:18: class ProfileLoader {
On 2013/06/13 15:10:16, sail wrote:
> Needs class comments describing why someone might want to use this.

Done.

https://codereview.chromium.org/16766003/diff/21001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_loader.h:23: bool AnyProfilesLoading() const;
On 2013/06/13 15:10:16, sail wrote:
> How about IsAnyProfileLoading() ?

Done.

[sail, 2013-06-14 17:05:46]

profiles/* LGTM

https://codereview.chromium.org/16766003/diff/29001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_loader.h (right):

https://codereview.chromium.org/16766003/diff/29001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_loader.h:20: // the profile is ready. Only the
callback for the most recent load request is
Maybe mention that the callback is not called if loading the profile fails?

Also, this comment just says what the class does which you can see by reading
the code. What would be more helpful is to explain the "why". Why does this
class exist? Why would you want to use it?

[jackhou1, 2013-06-17 04:40:15]

https://codereview.chromium.org/16766003/diff/29001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_loader.h (right):

https://codereview.chromium.org/16766003/diff/29001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_loader.h:20: // the profile is ready. Only the
callback for the most recent load request is
On 2013/06/14 17:05:46, sail wrote:
> Maybe mention that the callback is not called if loading the profile fails?
> 
> Also, this comment just says what the class does which you can see by reading
> the code. What would be more helpful is to explain the "why". Why does this
> class exist? Why would you want to use it?

Done.

[jackhou1, 2013-06-17 04:41:23]

tapted, benwells, PTAL

[tapted, 2013-06-17 05:03:54]

Perhaps set BUG=168080, and a TEST= line mentioning the added unit test,
Otherwise lgtm

[benwells, 2013-06-18 05:21:54]

On 2013/06/17 05:03:54, tapted wrote:
> Perhaps set BUG=168080, and a TEST= line mentioning the added unit test,
> Otherwise lgtm

(I believe TEST= are instructions to testers)

lgtm.

[commit-bot: I haz the power, 2013-06-18 06:47:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/16766003/51001

[jackhou1, 2013-06-18 06:47:28]

On 2013/06/18 05:21:54, benwells wrote:
> On 2013/06/17 05:03:54, tapted wrote:
> > Perhaps set BUG=168080, and a TEST= line mentioning the added unit test,
> > Otherwise lgtm
> 
> (I believe TEST= are instructions to testers)
> 
> lgtm.

Updated the description.

[commit-bot: I haz the power, 2013-06-18 07:05:47]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[jackhou1, 2013-06-19 06:58:18]

sail, benwells, PTAL at the #ifdef for android.

[benwells, 2013-06-19 09:13:44]

lgtm.

Another option would be to gyp-def it out on android, but the way you've done it
seems to follow the established pattern better, and I prefer it too (it is more
obvious being in an ifdef). However, sail@ probably knows more about the best
way to do it and might disagree.

[sail, 2013-06-19 19:44:28]

On 2013/06/19 09:13:44, benwells wrote:
> lgtm.
> 
> Another option would be to gyp-def it out on android, but the way you've done
it
> seems to follow the established pattern better, and I prefer it too (it is
more
> obvious being in an ifdef). However, sail@ probably knows more about the best
> way to do it and might disagree.

Yea, ifdefs should be avoided. Excluding this file from the android build would
be ok. Alternatively do a feature check to make the decision.

[jackhou1, 2013-06-20 10:09:27]

sail, PTAL

[sail, 2013-06-20 15:32:23]

lgtm

[commit-bot: I haz the power, 2013-06-20 23:13:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/16766003/91002

[commit-bot: I haz the power, 2013-06-21 07:15:48]

Change committed as 207745