Improve ephemeral profiles clean up code.

Improve ephemeral profiles clean up code.

BUG=302461
TEST=No new ones. Existing tests cover this code well enough.

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

[pastarmovj, 2013-11-04 15:20:48]

Hi,
Please review this CL.
Thanks!
Julian

https://codereview.chromium.org/50293011/diff/50001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/50293011/diff/50001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:878: if (profile) {
This is not related directly to what the rest of the CL does but I figured out
this is a bug to try to call profile->GetPath on potential NULL pointer and
therefore I changed the code to avoid it.

[rpetterson, 2013-11-04 18:18:58]

LGTM with just a question

https://codereview.chromium.org/50293011/diff/50001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/50293011/diff/50001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:1054: // to delay browser start-up.
Was there noticeable slow down for startup without the delay?

[pastarmovj, 2013-11-05 08:58:16]

Answered the question.

https://codereview.chromium.org/50293011/diff/50001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/50293011/diff/50001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:1054: // to delay browser start-up.
On 2013/11/04 18:18:59, rpetterson wrote:
> Was there noticeable slow down for startup without the delay?

To be honest I have never tried. Since I need to post the task to the FILE
thread anyways I thought I will also make it late enough to make sure no
start-up tasks are blocked on deleting potentially large amount of files.
Especially since Windows is known to be slow deleting many small files.

[jochen (gone - plz use gerrit), 2013-11-05 13:03:25]

https://codereview.chromium.org/50293011/diff/50001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/50293011/diff/50001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:1054: // to delay browser start-up.
On 2013/11/05 08:58:17, pastarmovj wrote:
> On 2013/11/04 18:18:59, rpetterson wrote:
> > Was there noticeable slow down for startup without the delay?
> 
> To be honest I have never tried. Since I need to post the task to the FILE
> thread anyways I thought I will also make it late enough to make sure no
> start-up tasks are blocked on deleting potentially large amount of files.
> Especially since Windows is known to be slow deleting many small files.

I don't think we should do that, unless you have data to show that this actually
slows down startup, and delaying it by 10 seconds does not.

https://codereview.chromium.org/50293011/diff/50001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/50293011/diff/50001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:169: void NukeProfileFromDisk(const
base::FilePath& profile) {
profile_path

https://codereview.chromium.org/50293011/diff/50001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/50293011/diff/50001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.h:193: static void
CleanUpStaleProfiles(std::vector<base::FilePath> profiles);
why not const ref?

I think profile_paths would be clearer

[pastarmovj, 2013-11-05 16:11:02]

Addressed the comments. Thanks for the reviews to you too!

https://codereview.chromium.org/50293011/diff/50001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/50293011/diff/50001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:1054: // to delay browser start-up.
On 2013/11/05 13:03:25, jochen wrote:
> On 2013/11/05 08:58:17, pastarmovj wrote:
> > On 2013/11/04 18:18:59, rpetterson wrote:
> > > Was there noticeable slow down for startup without the delay?
> > 
> > To be honest I have never tried. Since I need to post the task to the FILE
> > thread anyways I thought I will also make it late enough to make sure no
> > start-up tasks are blocked on deleting potentially large amount of files.
> > Especially since Windows is known to be slow deleting many small files.
> 
> I don't think we should do that, unless you have data to show that this
actually
> slows down startup, and delaying it by 10 seconds does not.

After discussing this with Jochen. I can see the point of not waiting here until
we gather evidence that this is indeed needed.

https://codereview.chromium.org/50293011/diff/50001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/50293011/diff/50001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.h:193: static void
CleanUpStaleProfiles(std::vector<base::FilePath> profiles);
On 2013/11/05 13:03:25, jochen wrote:
> why not const ref?
> 
> I think profile_paths would be clearer

Done.

[jochen (gone - plz use gerrit), 2013-11-05 16:50:37]

lgtm with nit addressed

https://codereview.chromium.org/50293011/diff/50001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/50293011/diff/50001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:169: void NukeProfileFromDisk(const
base::FilePath& profile) {
On 2013/11/05 13:03:25, jochen wrote:
> profile_path

What about this?

[pastarmovj, 2013-11-06 09:49:39]

https://codereview.chromium.org/50293011/diff/50001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/50293011/diff/50001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:169: void NukeProfileFromDisk(const
base::FilePath& profile) {
On 2013/11/05 16:50:38, jochen wrote:
> On 2013/11/05 13:03:25, jochen wrote:
> > profile_path
> 
> What about this?

Sorry I renamed the one below but skipped this one. Done!

[commit-bot: I haz the power, 2013-11-06 09:50:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/50293011/180001

[commit-bot: I haz the power, 2013-11-06 13:31:00]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-11-06 16:40:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/50293011/180001

[commit-bot: I haz the power, 2013-11-06 20:30:17]

Change committed as 233341