Remove deprecated extension notification from AppLoadService

Remove deprecated extension notification from AppLoadService

BUG=354046
Committed: https://crrev.com/d37917586956816bf0a0b0f02e68d7b2998e412e
Cr-Commit-Position: refs/heads/master@{#295186}

[limasdf, 2014-08-04 06:16:48]

Hi. tapted, could you take a look at this?

[tapted, 2014-08-04 07:05:38]

nice! just a minor gripe about ScopedObserver..

https://codereview.chromium.org/441553002/diff/1/apps/app_load_service.cc
File apps/app_load_service.cc (right):

https://codereview.chromium.org/441553002/diff/1/apps/app_load_service.cc#new...
apps/app_load_service.cc:43: AppLoadService::~AppLoadService() {}
This class is simple enough to just put

extensions::ExtensionRegistry::Get(profile_)->RemoveObserver(this);

here in the destructor. ScopedObserver is good if you observe multiple things of
the same type, but that's not needed here. We just get extra bloat and header
pollution.

[limasdf, 2014-08-04 07:30:35]

Thanks for the review!
PTAL

https://codereview.chromium.org/441553002/diff/1/apps/app_load_service.cc
File apps/app_load_service.cc (right):

https://codereview.chromium.org/441553002/diff/1/apps/app_load_service.cc#new...
apps/app_load_service.cc:43: AppLoadService::~AppLoadService() {}
On 2014/08/04 07:05:37, tapted wrote:
> This class is simple enough to just put
> 
> extensions::ExtensionRegistry::Get(profile_)->RemoveObserver(this);
> 
> here in the destructor. ScopedObserver is good if you observe multiple things
of
> the same type, but that's not needed here. We just get extra bloat and header
> pollution.

Done.

[tapted, 2014-08-04 07:54:05]

Thanks - lgtm

[limasdf, 2014-08-04 07:56:47]

The CQ bit was checked by limasdf@gmail.com

[commit-bot: I haz the power, 2014-08-04 07:57:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/limasdf@gmail.com/441553002/20001

[limasdf, 2014-08-04 08:31:16]

The CQ bit was unchecked by limasdf@gmail.com

[limasdf, 2014-08-04 11:29:25]

The CQ bit was checked by limasdf@gmail.com

[commit-bot: I haz the power, 2014-08-04 11:29:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/limasdf@gmail.com/441553002/40001

[tapted, 2014-08-04 12:53:20]

The CQ bit was unchecked by tapted@chromium.org

[tapted, 2014-08-04 12:55:13]

Can you explain why that change fixes the failing tests?

It's unit tests that are failing - ones that use TestingProfile. Perhaps there
is a dependency problem in testing_profile.cc that needs fixing.

[limasdf, 2014-08-04 13:51:26]

On 2014/08/04 12:55:13, tapted wrote:
> Can you explain why that change fixes the failing tests?
> 
> It's unit tests that are failing - ones that use TestingProfile. Perhaps there
> is a dependency problem in testing_profile.cc that needs fixing.

First of all. Sorry that I checked commit button without notifying.(I was
thinking there's same pattern so it is correct & tiny change)

The reason why PatchSet2 failed is ExtesnionRegistry::Get(incongnito_profile) 
between Constructor and Destructor has different value.
See the result https://codereview.chromium.org/436583006/

I'm not sure if this is intended or not. maybe @kalman can give us wisdom.
Or please give me more time to investigate.

[limasdf, 2014-08-04 13:51:51]

added kalman for the wisdom

[limasdf, 2014-08-04 14:38:48]

OK. the problem is profile_->GetOriginalProfile() from
AppLoadService::AppLoadService can NOT return OriginalProfile. so the registry
also has different value.

I'll investigate it. Sorry to bother you

[not at google - send to devlin, 2014-08-04 15:47:55]

On 2014/08/04 14:38:48, limasdf wrote:
> OK. the problem is profile_->GetOriginalProfile() from
> AppLoadService::AppLoadService can NOT return OriginalProfile. so the registry
> also has different value.

The ExtensionRegistry should be shared between the normal and incognito
profiles, so it should have the same value either way.

i.e.

DCHECK_EQ(
    ExtensionRegistry::Get(profile->GetOriginalProfile()),
    ExtensionRegistry::Get(profile->GetOffTheRecordProfile());

will pass.

[tapted, 2014-08-05 01:15:59]

On 2014/08/04 15:47:55, kalman wrote:
> On 2014/08/04 14:38:48, limasdf wrote:
> > OK. the problem is profile_->GetOriginalProfile() from
> > AppLoadService::AppLoadService can NOT return OriginalProfile. so the
registry
> > also has different value.

I suspect the problem is with TestingProfile::GetOffTheRecordProfile(), and
you've managed to expose a bug in our testing infrastructure.

That function does

    TestingProfile::Builder builder;
    builder.SetIncognito();
    scoped_ptr<TestingProfile> incognito_test_profile(builder.Build());
    incognito_test_profile->SetOriginalProfile(this);

The last line -- SetOriginalProfile -- changes the return of
GetOriginalProfile(). But it's being done *after* the profile is built on the
previous line. I'm guessing it needs to be done beforehand in order for the
tests to be sane. Do you feel like investigating further?

[limasdf, 2014-08-06 01:24:38]

On 2014/08/05 01:15:59, tapted wrote:
> On 2014/08/04 15:47:55, kalman wrote:
> > On 2014/08/04 14:38:48, limasdf wrote:
> > > OK. the problem is profile_->GetOriginalProfile() from
> > > AppLoadService::AppLoadService can NOT return OriginalProfile. so the
> registry
> > > also has different value.
> 
> I suspect the problem is with TestingProfile::GetOffTheRecordProfile(), and
> you've managed to expose a bug in our testing infrastructure.
> 
> That function does
> 
>     TestingProfile::Builder builder;
>     builder.SetIncognito();
>     scoped_ptr<TestingProfile> incognito_test_profile(builder.Build());
>     incognito_test_profile->SetOriginalProfile(this);
> 
> The last line -- SetOriginalProfile -- changes the return of
> GetOriginalProfile(). But it's being done *after* the profile is built on the
> previous line. I'm guessing it needs to be done beforehand in order for the
> tests to be sane. Do you feel like investigating further?

Yeah. I agree that it need to be done first.
I'll investigating futher. and then Maybe I'm going to file an issue about it.
If there's more question, please allow me to ask you about that.

[not at google - send to devlin, 2014-08-06 17:16:52]

On 2014/08/06 01:24:38, limasdf wrote:
> On 2014/08/05 01:15:59, tapted wrote:
> > On 2014/08/04 15:47:55, kalman wrote:
> > > On 2014/08/04 14:38:48, limasdf wrote:
> > > > OK. the problem is profile_->GetOriginalProfile() from
> > > > AppLoadService::AppLoadService can NOT return OriginalProfile. so the
> > registry
> > > > also has different value.
> > 
> > I suspect the problem is with TestingProfile::GetOffTheRecordProfile(), and
> > you've managed to expose a bug in our testing infrastructure.
> > 
> > That function does
> > 
> >     TestingProfile::Builder builder;
> >     builder.SetIncognito();
> >     scoped_ptr<TestingProfile> incognito_test_profile(builder.Build());
> >     incognito_test_profile->SetOriginalProfile(this);
> > 
> > The last line -- SetOriginalProfile -- changes the return of
> > GetOriginalProfile(). But it's being done *after* the profile is built on
the
> > previous line. I'm guessing it needs to be done beforehand in order for the
> > tests to be sane. Do you feel like investigating further?
> 
> Yeah. I agree that it need to be done first.
> I'll investigating futher. and then Maybe I'm going to file an issue about it.
> If there's more question, please allow me to ask you about that.

Thanks!

[tapted, 2014-09-16 01:20:52]

If you want to give patchset 2 another go, it should now pass. You'll need to
rebase it (so not lgtm yet), since app_load_service.cc has changed.

[limasdf, 2014-09-16 06:59:46]

On 2014/09/16 01:20:52, tapted wrote:
> If you want to give patchset 2 another go, it should now pass. You'll need to
> rebase it (so not lgtm yet), since app_load_service.cc has changed.

Thank you very much for fixing testing profile issue.
After back to patch set 2, there's one fail in test.
I'll investigate it tonight.
Thank you again!

[limasdf, 2014-09-16 10:32:05]

PTAL!

[tapted, 2014-09-16 21:52:57]

lgtm!

[limasdf, 2014-09-16 22:37:30]

The CQ bit was checked by limasdf@gmail.com

[commit-bot: I haz the power, 2014-09-16 22:38:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/441553002/80001

[limasdf, 2014-09-16 22:46:43]

On 2014/09/16 22:38:27, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patchset/441553002/80001

Thank a ton!
As a contributor, your fix the blocking issue(which is hard for me) is big help!

[commit-bot: I haz the power, 2014-09-17 00:22:02]

Committed patchset #5 (id:80001) as cf83a7a9295c907eea26e1c7e8103797c4ac3ca3

[commit-bot: I haz the power, 2014-09-17 00:22:28]

Patchset 5 (id:??) landed as
https://crrev.com/d37917586956816bf0a0b0f02e68d7b2998e412e
Cr-Commit-Position: refs/heads/master@{#295186}