Refactor utility methods out of the ProfileManager

Refactor utility methods that don't use the ProfileManager directly out of the ProfileManager.

I only picked public, static, functions that look standalone
-- i.e. don't use any other functions in the ProfileManager,
and only interact with other classes (browser, file paths,
registry preferences)

BUG=none
TEST=existing unit and browser tests

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

[noms, 2013-07-08 19:25:40]

Hi Roger & Sailesh,

I've refactored the standalone methods out of the ProfileManager (i.e. the ones
that don't actually touch the ProfileManager class). Would you mind taking a
look?

Thanks!

[tfarina, 2013-07-08 19:42:51]

https://codereview.chromium.org/18615010/diff/13001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager_util.h (right):

https://codereview.chromium.org/18615010/diff/13001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager_util.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
micronit: no (c)

https://codereview.chromium.org/18615010/diff/13001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager_util.h:9: #include
"chrome/browser/profiles/profile.h"
could you forward declare this instead?

https://codereview.chromium.org/18615010/diff/13001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager_util.h:26: void
RegisterPrefs(PrefRegistrySimple* registry);
please forward declare this, don't rely on the includes above.

[noms, 2013-07-08 21:44:47]

https://codereview.chromium.org/18615010/diff/13001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager_util.h (right):

https://codereview.chromium.org/18615010/diff/13001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager_util.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2013/07/08 19:42:51, tfarina wrote:
> micronit: no (c)

Done.

https://codereview.chromium.org/18615010/diff/13001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager_util.h:9: #include
"chrome/browser/profiles/profile.h"
On 2013/07/08 19:42:51, tfarina wrote:
> could you forward declare this instead?

Done.

https://codereview.chromium.org/18615010/diff/13001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager_util.h:26: void
RegisterPrefs(PrefRegistrySimple* registry);
On 2013/07/08 19:42:51, tfarina wrote:
> please forward declare this, don't rely on the includes above.

Done.

[Roger Tawa OOO till Jul 10th, 2013-07-09 14:00:43]

lgtm

[sail, 2013-07-09 14:27:27]

Hi, I'll be away on leave for a few weeks. Maybe erg@ or mirandac@ might be a
good person to review this CL. Thanks.

[noms, 2013-07-09 14:31:39]

Hi Miranda, would you mind taking a look at this CL? Thanks!

[Miranda Callahan, 2013-07-09 14:36:28]

On 2013/07/09 14:31:39, noms wrote:
> Hi Miranda, would you mind taking a look at this CL? Thanks!

Hi, noms -- sure, will take a look now.

[Miranda Callahan, 2013-07-09 14:49:32]

LGTM with a couple includes that can be deleted. Great work!

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

https://codereview.chromium.org/18615010/diff/15002/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.h:24: #include
"chrome/browser/ui/host_desktop.h"
You can probably remove host_desktop here.

https://codereview.chromium.org/18615010/diff/15002/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.h:25: #include
"chrome/browser/ui/startup/startup_types.h"
Looks like you may be able to remove startup_types as well.

[noms, 2013-07-09 15:36:04]

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

https://codereview.chromium.org/18615010/diff/15002/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.h:24: #include
"chrome/browser/ui/host_desktop.h"
On 2013/07/09 14:49:33, Miranda Callahan wrote:
> You can probably remove host_desktop here.

Done.

https://codereview.chromium.org/18615010/diff/15002/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.h:25: #include
"chrome/browser/ui/startup/startup_types.h"
On 2013/07/09 14:49:33, Miranda Callahan wrote:
> Looks like you may be able to remove startup_types as well.

Done.

[tfarina, 2013-07-09 16:10:45]

https://codereview.chromium.org/18615010/diff/28001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager_util.h (right):

https://codereview.chromium.org/18615010/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager_util.h:5: #ifndef
CHROME_BROWSER_PROFILES_PROFILE_MANAGER_UTIL_H_
is this super related to ProfileManager? if not, I'd rename this to just
profile_utils.h

https://codereview.chromium.org/18615010/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager_util.h:12: class FilePath;
don't indent two spaces inside namespace declaration. 

clang-format allows you to format it like this too:

namespace base { class FilePath; }

also move this below class Profile;

[tfarina, 2013-07-09 16:10:45]

https://codereview.chromium.org/18615010/diff/28001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager_util.h (right):

https://codereview.chromium.org/18615010/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager_util.h:5: #ifndef
CHROME_BROWSER_PROFILES_PROFILE_MANAGER_UTIL_H_
is this super related to ProfileManager? if not, I'd rename this to just
profile_utils.h

https://codereview.chromium.org/18615010/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager_util.h:12: class FilePath;
don't indent two spaces inside namespace declaration. 

clang-format allows you to format it like this too:

namespace base { class FilePath; }

also move this below class Profile;

[noms, 2013-07-09 17:47:39]

+ Nico for top-level chrome/ owner stamp
+ Greg for chrome_frame/test owner stamp
(for context: I moved some files out of ProfileManager and into a util class.
@mirandac owns that API, and she reviewed it. All the other classes were just
updated to use the new API. Could you please take a look? Thanks!)

https://codereview.chromium.org/18615010/diff/28001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager_util.h (right):

https://codereview.chromium.org/18615010/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager_util.h:5: #ifndef
CHROME_BROWSER_PROFILES_PROFILE_MANAGER_UTIL_H_
These are methods that used to be in the ProfileManager. In the future I'd like
to refactor more out of that class, if possible, so I think it might make sense
if it's a ProfileManager related class.

On 2013/07/09 16:10:46, tfarina wrote:
> is this super related to ProfileManager? if not, I'd rename this to just
> profile_utils.h

https://codereview.chromium.org/18615010/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager_util.h:12: class FilePath;
On 2013/07/09 16:10:46, tfarina wrote:
> don't indent two spaces inside namespace declaration. 
> 
> clang-format allows you to format it like this too:
> 
> namespace base { class FilePath; }
> 
> also move this below class Profile;

Done.

[Nico, 2013-07-09 17:58:06]

We discourage _util files, see this (badly titled) thread:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/cb2KPb0KG40

Can you think of a more specific name?

(code changes themselves look fine)

[noms, 2013-07-09 20:37:13]

The only other name I can think of is _helpers, which I think might be just as
uninspiring. The file contains an assortment of misc functions that are
tangentially related to the ProfileManager (in the sense that they help with
managing of profiles), but have nothing in common between them (a function opens
a browser window, another sets a preference).  

I agree that as a general policy _util should be discouraged, but I don't have a
better name in this case. :(
I am open to all suggestions, though.

On 2013/07/09 17:58:06, Nico wrote:
> We discourage _util files, see this (badly titled) thread:
>
https://groups.google.com/a/chromium.org/forum/#%21topic/chromium-dev/cb2KPb0...
> 
> Can you think of a more specific name?
> 
> (code changes themselves look fine)

[noms, 2013-07-11 15:01:15]

s/grt/erikwright/ for chrome_frame/test owner stamp, as Greg is out of office :)

[erikwright (departed), 2013-07-11 15:02:18]

chrome_frame/ LGTM

[noms, 2013-07-11 17:16:38]

Nico: ping

[noms, 2013-07-15 14:35:33]

Nico: pong? :)
On 2013/07/11 17:16:38, noms wrote:
> Nico: ping

[Nico, 2013-07-15 23:54:29]

https://codereview.chromium.org/18615010/diff/48001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager_util.h (right):

https://codereview.chromium.org/18615010/diff/48001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager_util.h:28: void
RegisterPrefs(PrefRegistrySimple* registry);
I'd go with profiles_state.h for the functions above this comment, and
profile_window for the function below.

[noms, 2013-07-16 18:10:40]

https://codereview.chromium.org/18615010/diff/48001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager_util.h (right):

https://codereview.chromium.org/18615010/diff/48001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager_util.h:28: void
RegisterPrefs(PrefRegistrySimple* registry);
On 2013/07/15 23:54:30, Nico wrote:
> I'd go with profiles_state.h for the functions above this comment, and
> profile_window for the function below.

Done.

[tfarina, 2013-07-16 18:15:03]

https://codereview.chromium.org/18615010/diff/58001/chrome/browser/profiles/p...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/18615010/diff/58001/chrome/browser/profiles/p...
chrome/browser/profiles/profiles_state.cc:10: #include
"chrome/browser/profiles/profile.h"
can you remove this?

https://codereview.chromium.org/18615010/diff/58001/chrome/browser/profiles/p...
File chrome/browser/profiles/profiles_state.h (right):

https://codereview.chromium.org/18615010/diff/58001/chrome/browser/profiles/p...
chrome/browser/profiles/profiles_state.h:9: class Profile;
rm this.

[noms, 2013-07-16 18:24:02]

https://codereview.chromium.org/18615010/diff/58001/chrome/browser/profiles/p...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/18615010/diff/58001/chrome/browser/profiles/p...
chrome/browser/profiles/profiles_state.cc:10: #include
"chrome/browser/profiles/profile.h"
On 2013/07/16 18:15:04, tfarina wrote:
> can you remove this?

Done.

https://codereview.chromium.org/18615010/diff/58001/chrome/browser/profiles/p...
File chrome/browser/profiles/profiles_state.h (right):

https://codereview.chromium.org/18615010/diff/58001/chrome/browser/profiles/p...
chrome/browser/profiles/profiles_state.h:9: class Profile;
Argh. Missed this one.
Done.
On 2013/07/16 18:15:04, tfarina wrote:
> rm this.

[Nico, 2013-07-16 19:41:24]

lgtm

[commit-bot: I haz the power, 2013-07-16 20:14:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/18615010/81001

[commit-bot: I haz the power, 2013-07-17 05:11:55]

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

[commit-bot: I haz the power, 2013-07-17 13:25:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/18615010/81001

[commit-bot: I haz the power, 2013-07-17 13:25:08]

Failed to apply patch for chrome/browser/profiles/avatar_menu_model.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/profiles/avatar_menu_model.cc
  Hunk #4 FAILED at 271.
  1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/profiles/avatar_menu_model.cc.rej

Patch:       chrome/browser/profiles/avatar_menu_model.cc
Index: chrome/browser/profiles/avatar_menu_model.cc
diff --git a/chrome/browser/profiles/avatar_menu_model.cc
b/chrome/browser/profiles/avatar_menu_model.cc
index
f05dd2f1f89720484af20c7c79aa94a5ea9362ec..aa8e6c055ea746294b78d233246ac972c238edf1
100644
--- a/chrome/browser/profiles/avatar_menu_model.cc
+++ b/chrome/browser/profiles/avatar_menu_model.cc
@@ -18,6 +18,8 @@
 #include "chrome/browser/profiles/profile_info_util.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/profiles/profile_metrics.h"
+#include "chrome/browser/profiles/profile_window.h"
+#include "chrome/browser/profiles/profiles_state.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_window.h"
@@ -51,7 +53,7 @@ void OnProfileCreated(bool always_create,
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (status == Profile::CREATE_STATUS_INITIALIZED) {
-    ProfileManager::FindOrCreateNewWindowForProfile(
+    profiles::FindOrCreateNewWindowForProfile(
         profile,
         chrome::startup::IS_NOT_PROCESS_STARTUP,
         chrome::startup::IS_NOT_FIRST_RUN,
@@ -140,7 +142,7 @@ AvatarMenuModel::Item::~Item() {
 }
 
 void AvatarMenuModel::SwitchToProfile(size_t index, bool always_create) {
-  DCHECK(ProfileManager::IsMultipleProfilesEnabled() ||
+  DCHECK(profiles::IsMultipleProfilesEnabled() ||
          index == GetActiveProfileIndex());
   const Item& item = GetItemAt(index);
   base::FilePath path =
@@ -269,10 +271,10 @@ bool AvatarMenuModel::ShouldShowAvatarMenu() {
   if (base::FieldTrialList::FindFullName(kShowProfileSwitcherFieldTrialName) ==
       kAlwaysShowSwitcherGroupName) {
     // We should only be in this group when multi-profiles is enabled.
-    DCHECK(ProfileManager::IsMultipleProfilesEnabled());
+    DCHECK(profiles::IsMultipleProfilesEnabled());
     return true;
   }
-  return ProfileManager::IsMultipleProfilesEnabled() &&
+  return profiles::IsMultipleProfilesEnabled() &&
       g_browser_process->profile_manager() &&
       g_browser_process->profile_manager()->GetNumberOfProfiles() > 1;
 }

[commit-bot: I haz the power, 2013-07-17 15:50:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/18615010/106001

[commit-bot: I haz the power, 2013-07-18 03:30:41]

Change committed as 212221