Replace ProfileManagerObserver with callback

Replace ProfileManagerObserver with callback

The old ProfileManagerObserver was hard to use since it required a new class or subclassing. Using callbacks is easier.

BUG=100461
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113480

[Robert Sesek, 2011-12-07 16:36:12]

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_performer.h (right):

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_performer.h:55: public
base::SupportsWeakPtr<LoginUtilsImpl> {
This class already holds a WeakPtrFactory weak_factory_. I don't think you need
this (also, this seems like the wrong template param).

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

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:357: callback);
nit: Does this fit on one line now?

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:496: if (success &
!go_off_the_record) {
&&

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:501: if (success) {
I think this if-block structure is less clear than the nested if's. At first
glance, it seems like this should be an else if.

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:514: callbacks[i].Run(profile,
Profile::CREATE_STATUS_CREATED);
What's the point of the CREATED status? It seems like if you send INITIALIZED
for all profiles right after this, CREATED doesn't do much.

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

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.h:74: CreateCallback callback);
const ref

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.h:79: static void
CreateDefaultProfileAsync(CreateCallback callback);
const ref

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager_unittest.cc:119: class MockObserver :
public base::RefCountedThreadSafe<MockObserver> {
Why does this need to be refcounted?

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager_unittest.cc:121:
MOCK_METHOD2(OnProfileCreated, void(Profile* profile,
I'd break after the first ,

[sail, 2011-12-07 17:18:13]

Thanks for the super quick review!

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_performer.h (right):

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_performer.h:55: public
base::SupportsWeakPtr<LoginUtilsImpl> {
On 2011/12/07 16:36:13, rsesek wrote:
> This class already holds a WeakPtrFactory weak_factory_. I don't think you
need
> this (also, this seems like the wrong template param).

Done.

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

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:357: callback);
On 2011/12/07 16:36:13, rsesek wrote:
> nit: Does this fit on one line now?

Done.

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:496: if (success &
!go_off_the_record) {
On 2011/12/07 16:36:13, rsesek wrote:
> &&

Done.

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:501: if (success) {
On 2011/12/07 16:36:13, rsesek wrote:
> I think this if-block structure is less clear than the nested if's. At first
> glance, it seems like this should be an else if.

I found the original code very confusing which is why I change the structure a
bit. I agree the new code isn't perfect but I find it much easier to break down
into logical groups. I added a comment here to help out.

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:514: callbacks[i].Run(profile,
Profile::CREATE_STATUS_CREATED);
On 2011/12/07 16:36:13, rsesek wrote:
> What's the point of the CREATED status? It seems like if you send INITIALIZED
> for all profiles right after this, CREATED doesn't do much.

It looks like CREATED was added for the ChromeOS code. It needs to be called
before DoFinalInit() is called.

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

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.h:74: CreateCallback callback);
On 2011/12/07 16:36:13, rsesek wrote:
> const ref

Done.

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.h:79: static void
CreateDefaultProfileAsync(CreateCallback callback);
On 2011/12/07 16:36:13, rsesek wrote:
> const ref

Done.

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager_unittest.cc:119: class MockObserver :
public base::RefCountedThreadSafe<MockObserver> {
On 2011/12/07 16:36:13, rsesek wrote:
> Why does this need to be refcounted?

So I can use it in base::Bind().

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager_unittest.cc:121:
MOCK_METHOD2(OnProfileCreated, void(Profile* profile,
On 2011/12/07 16:36:13, rsesek wrote:
> I'd break after the first ,

Done.

[sail, 2011-12-07 17:20:52]

altimofeev, avayvod: chromeos/login OWNERS approval

[whywhat, 2011-12-07 17:35:50]

altimofeev is on vacation :)

lgtm with some nits:

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

http://codereview.chromium.org/8840003/diff/9001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:332: // Profile has already been
created. Call observer immediately.
nit: s/Call observer/Run callback

http://codereview.chromium.org/8840003/diff/9001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:518: for (size_t i = 0; i <
callbacks.size(); ++i) {
nit: move the loop to run all callbacks to a method?

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

http://codereview.chromium.org/8840003/diff/9001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.h:72: // immediately.
nit: Please, leave "Should be called on the UI thread" here if still applicable.

http://codereview.chromium.org/8840003/diff/9001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.h:215: // List of callbacks which should
be notified when profile initialization is
nit: s/which should be notified/to run

[Elliot Glaysher, 2011-12-07 17:43:11]

lgtm

[Robert Sesek, 2011-12-07 18:07:39]

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager_unittest.cc:119: class MockObserver :
public base::RefCountedThreadSafe<MockObserver> {
On 2011/12/07 17:18:13, sail wrote:
> On 2011/12/07 16:36:13, rsesek wrote:
> > Why does this need to be refcounted?
> 
> So I can use it in base::Bind().

Can't you just use Unretained() because the MockObserver is limited to the scope
of each test?

[sail, 2011-12-07 18:34:04]

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

http://codereview.chromium.org/8840003/diff/1010/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager_unittest.cc:119: class MockObserver :
public base::RefCountedThreadSafe<MockObserver> {
On 2011/12/07 18:07:40, rsesek wrote:
> On 2011/12/07 17:18:13, sail wrote:
> > On 2011/12/07 16:36:13, rsesek wrote:
> > > Why does this need to be refcounted?
> > 
> > So I can use it in base::Bind().
> 
> Can't you just use Unretained() because the MockObserver is limited to the
scope
> of each test?

Done.

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

http://codereview.chromium.org/8840003/diff/9001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:332: // Profile has already been
created. Call observer immediately.
On 2011/12/07 17:35:50, whywhat wrote:
> nit: s/Call observer/Run callback

Done.

http://codereview.chromium.org/8840003/diff/9001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:518: for (size_t i = 0; i <
callbacks.size(); ++i) {
On 2011/12/07 17:35:50, whywhat wrote:
> nit: move the loop to run all callbacks to a method?

Done.

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

http://codereview.chromium.org/8840003/diff/9001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.h:72: // immediately.
On 2011/12/07 17:35:50, whywhat wrote:
> nit: Please, leave "Should be called on the UI thread" here if still
applicable.

Done.

http://codereview.chromium.org/8840003/diff/9001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.h:215: // List of callbacks which should
be notified when profile initialization is
On 2011/12/07 17:35:50, whywhat wrote:
> nit: s/which should be notified/to run

Done.

[Robert Sesek, 2011-12-07 19:34:47]

lgtm

[commit-bot: I haz the power, 2011-12-07 20:09:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/8840003/12004

[commit-bot: I haz the power, 2011-12-07 22:14:02]

Change committed as 113480