Multi-Profiles: Add delete profile command

Multi-Profiles: Add delete profile command

This change adds a delete profile command. The command does the following:
   - closes all browser windows with that profile
   - removes the profile from our list of known profiles
   - schedules the profile directory to be deleted when the application quits

The command will eventually be exposed through the settings UI. Until that's ready I'm adding it to the avatar menu.

BUG=None
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91777

[willchan no longer on Chromium, 2011-07-07 17:36:27]

I defer to mirandac here.

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:46: void
DeleteProfileDirectories(std::vector<FilePath> paths) {
const std::vector<FilePath>&

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:115: if (profiles_to_delete_.size())
{
Perhaps we should tag this with TODO: fix crbug.com/88586.

One problem with waiting until shutdown like this is it delays browser process
shutdown and prevents restarting chrome until it finishes.

[Miranda Callahan, 2011-07-07 17:43:02]

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:561:
BrowserList::CloseAllBrowsersWithProfile(profile);
Is there any issue with BackgroundApps that we have to consider here?

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager.h (right):

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.h:222: std::vector<FilePath>
profiles_to_delete_;
Does this need to be made thread-safe?

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/ui/profile_me...
File chrome/browser/ui/profile_menu_model.cc (right):

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/ui/profile_me...
chrome/browser/ui/profile_menu_model.cc:47:
g_browser_process->profile_manager()->ScheduleProfileForDeletion(
We probably need to stop background apps for this profile from running at this
time as well.

[Miranda Callahan, 2011-07-07 17:45:54]

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:115: if (profiles_to_delete_.size())
{
On 2011/07/07 17:36:27, willchan wrote:
> Perhaps we should tag this with TODO: fix crbug.com/88586.
> 
> One problem with waiting until shutdown like this is it delays browser process
> shutdown and prevents restarting chrome until it finishes.

Agreed wholeheartedly about tagging; another reason to get Profile shutdown
working nicely soon.

[sail, 2011-07-07 18:00:19]

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:46: void
DeleteProfileDirectories(std::vector<FilePath> paths) {
On 2011/07/07 17:36:27, willchan wrote:
> const std::vector<FilePath>&

To make it thread safe I want this to be a copy of the vector. This is vector is
going to be really small so it shouldn't be a performance hit.

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:115: if (profiles_to_delete_.size())
{
On 2011/07/07 17:36:27, willchan wrote:
> Perhaps we should tag this with TODO: fix crbug.com/88586.
> 
> One problem with waiting until shutdown like this is it delays browser process
> shutdown and prevents restarting chrome until it finishes.

Done.

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:561:
BrowserList::CloseAllBrowsersWithProfile(profile);
On 2011/07/07 17:43:02, Miranda Callahan wrote:
> Is there any issue with BackgroundApps that we have to consider here?

No idea. Would rachel know?

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager.h (right):

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.h:222: std::vector<FilePath>
profiles_to_delete_;
On 2011/07/07 17:43:02, Miranda Callahan wrote:
> Does this need to be made thread-safe?

It doesn't because it' only accessed from the UI thread. When calling the FILE
IO thread we make a copy of the vector.

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/ui/profile_me...
File chrome/browser/ui/profile_menu_model.cc (right):

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/ui/profile_me...
chrome/browser/ui/profile_menu_model.cc:47:
g_browser_process->profile_manager()->ScheduleProfileForDeletion(
On 2011/07/07 17:43:02, Miranda Callahan wrote:
> We probably need to stop background apps for this profile from running at this
> time as well.

Sounds good. Mind if I do this as a separate change?

[Miranda Callahan, 2011-07-07 18:24:16]

LGTM with following change to shut down background apps, if Rachel agrees that
this is the correct thing to do.

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:561:
BrowserList::CloseAllBrowsersWithProfile(profile);
On 2011/07/07 18:00:19, sail wrote:
> On 2011/07/07 17:43:02, Miranda Callahan wrote:
> > Is there any issue with BackgroundApps that we have to consider here?
> 
> No idea. Would rachel know?

I can imagine that she would -- adding her in.

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/ui/profile_me...
File chrome/browser/ui/profile_menu_model.cc (right):

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/ui/profile_me...
chrome/browser/ui/profile_menu_model.cc:47:
g_browser_process->profile_manager()->ScheduleProfileForDeletion(
On 2011/07/07 18:00:19, sail wrote:
> On 2011/07/07 17:43:02, Miranda Callahan wrote:
> > We probably need to stop background apps for this profile from running at
this
> > time as well.
> 
> Sounds good. Mind if I do this as a separate change?

Sure, no problem -- then we can get Rachel in that change as well.

[rpetterson, 2011-07-07 18:24:30]

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:561:
BrowserList::CloseAllBrowsersWithProfile(profile);
On 2011/07/07 18:00:19, sail wrote:
> On 2011/07/07 17:43:02, Miranda Callahan wrote:
> > Is there any issue with BackgroundApps that we have to consider here?
> 
> No idea. Would rachel know?

This particular call shouldn't affect BackgroundApps since the actual profile
instance isn't deleted. But when it is deleted, we should make sure to handle it
correctly.

[Miranda Callahan, 2011-07-07 18:24:47]

On 2011/07/07 18:24:16, Miranda Callahan wrote:
> LGTM with following change to shut down background apps, if Rachel agrees that
> this is the correct thing to do.
> 
>
http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
> File chrome/browser/profiles/profile_manager.cc (right):
> 
>
http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
> chrome/browser/profiles/profile_manager.cc:561:
> BrowserList::CloseAllBrowsersWithProfile(profile);
> On 2011/07/07 18:00:19, sail wrote:
> > On 2011/07/07 17:43:02, Miranda Callahan wrote:
> > > Is there any issue with BackgroundApps that we have to consider here?
> > 
> > No idea. Would rachel know?
> 
> I can imagine that she would -- adding her in.
> 
>
http://codereview.chromium.org/7321011/diff/2001/chrome/browser/ui/profile_me...
> File chrome/browser/ui/profile_menu_model.cc (right):
> 
>
http://codereview.chromium.org/7321011/diff/2001/chrome/browser/ui/profile_me...
> chrome/browser/ui/profile_menu_model.cc:47:
> g_browser_process->profile_manager()->ScheduleProfileForDeletion(
> On 2011/07/07 18:00:19, sail wrote:
> > On 2011/07/07 17:43:02, Miranda Callahan wrote:
> > > We probably need to stop background apps for this profile from running at
> this
> > > time as well.
> > 
> > Sounds good. Mind if I do this as a separate change?
> 
> Sure, no problem -- then we can get Rachel in that change as well.

(Oh, and tryservers green, of course. :-)

[sail, 2011-07-07 18:29:55]

Added a TODO for background apps.

[willchan no longer on Chromium, 2011-07-08 08:33:17]

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:46: void
DeleteProfileDirectories(std::vector<FilePath> paths) {
On 2011/07/07 18:00:19, sail wrote:
> On 2011/07/07 17:36:27, willchan wrote:
> > const std::vector<FilePath>&
> 
> To make it thread safe I want this to be a copy of the vector. This is vector
is
> going to be really small so it shouldn't be a performance hit.

NewRunnableFunction() will take the parameters and copy them. See:

http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/task.h&exact_pac...
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/task.h&exact_pac...
and in particular:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/task.h&exact_pac...

The last link will show how for const A& a passed as a parameter to
NewRunnableFunction(), NewRunnableFunction() will create a Tuple1<A>, NOT
Tuple1<const A&>. This Tuple1<A> becomes the Params template parameter in the
previous links. The Params parameter becomes a member variable of the
RunnableFunction subclass of Task.

Contrary to popular misunderstanding, the function prototype does not affect
whether or not NewRunnableMethod()/NewRunnableFunction() copies the parameter
into the Task.

So making this function prototype be like this just causes
RunnableFunction::Run() to make an extra unnecessary copy of this vector when it
calls the function pointer.

[sail, 2011-07-08 16:04:33]

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/7321011/diff/2001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:46: void
DeleteProfileDirectories(std::vector<FilePath> paths) {
On 2011/07/08 08:33:17, willchan wrote:
> On 2011/07/07 18:00:19, sail wrote:
> > On 2011/07/07 17:36:27, willchan wrote:
> > > const std::vector<FilePath>&
> > 
> > To make it thread safe I want this to be a copy of the vector. This is
vector
> is
> > going to be really small so it shouldn't be a performance hit.
> 
> NewRunnableFunction() will take the parameters and copy them. See:
> 
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/task.h&exact_pac...
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/task.h&exact_pac...
> and in particular:
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/task.h&exact_pac...
> 
> The last link will show how for const A& a passed as a parameter to
> NewRunnableFunction(), NewRunnableFunction() will create a Tuple1<A>, NOT
> Tuple1<const A&>. This Tuple1<A> becomes the Params template parameter in the
> previous links. The Params parameter becomes a member variable of the
> RunnableFunction subclass of Task.
> 
> Contrary to popular misunderstanding, the function prototype does not affect
> whether or not NewRunnableMethod()/NewRunnableFunction() copies the parameter
> into the Task.
> 
> So making this function prototype be like this just causes
> RunnableFunction::Run() to make an extra unnecessary copy of this vector when
it
> calls the function pointer.

Thanks for the explanation! Sent out fix in a separate CL:
http://codereview.chromium.org/7328021