Implement loading blacklists from extensions....

Implement loading blacklists from extensions.

It doesn't yet work in full-browser scenario, but allows me
to write a simple test.

TEST=Covered by browser_tests.
BUG=21541
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30952

[sky, 2009-10-30 15:53:37]

Seems odd to have two notifications fired at the same time. Did you
consider changing EXTENSION_LOADED/UNLOADED to use the source as the
profile? Not knowing any of the code that seems like it would work as
you can get the extensionservice from the profile.

  -Scott

On Fri, Oct 30, 2009 at 8:14 AM,  <phajdan.jr@chromium.org> wrote:
> Reviewers: Aaron Boodman, sky,
>
> Message:
> aa, you probably won't be happy that I didn't use existing extension
> notifications for that. If you have a solution that would help with the
> problem
> I describe, I'll use it.
>
> The problem is testing. ExtensionsService is hard to mock or use in a uni=
t
> test
> (too heavy to use directly or even construct in a unit test). And in
> BlacklistManager I'd have to extract the ExtensionsService from Profile a=
nd
> use
> it as the Source of notification it listens to (because receiving
> notifications
> from other Profiles will cause spurious recompilations).
>
> Description:
> Implement loading blacklists from extensions.
>
> It doesn't yet work in full-browser scenario, but allows me
> to write a simple test.
>
> TEST=3DCovered by browser_tests.
> BUG=3D21541
>
> Please review this at http://codereview.chromium.org/341050
>
> SVN Base: http://src.chromium.org/svn/trunk/src/
>
> Affected files:
> =A0M =A0 =A0 chrome/browser/extensions/extensions_service.h
> =A0M =A0 =A0 chrome/browser/extensions/extensions_service.cc
> =A0A =A0 =A0 chrome/browser/privacy_blacklist/blacklist_manager_browserte=
st.cc
> =A0M =A0 =A0 chrome/chrome.gyp
> =A0A =A0 =A0 chrome/test/data/extensions/common/privacy_blacklist/manifes=
t.json
> =A0A
> chrome/test/data/extensions/common/privacy_blacklist/privacy_blacklist.pb=
l
>
>
>

[Paweł Hajdan Jr., 2009-10-30 16:51:48]

On 2009/10/30 15:53:37, sky wrote:
> Seems odd to have two notifications fired at the same time. Did you
> consider changing EXTENSION_LOADED/UNLOADED to use the source as the
> profile? Not knowing any of the code that seems like it would work as
> you can get the extensionservice from the profile.

I didn't think about it earlier, but sounds like a good choice to me and I can
do that. Aaron, what do you think?

[Aaron Boodman, 2009-11-01 00:20:37]

I'm OK making that change, but fwiw ES also has a profile_ pointer. You could
just add a getter for it. IMO, it is slightly more accurate to say that the
source of these notifications is ES, not Profile.

Other than that, LGTM, and sorry for the long review.

[Paweł Hajdan Jr., 2009-11-03 17:38:42]

Aaron, could you review the latest revision of this patch? It should fix the
unit_tests crash (and it was a Singleton issue!).

Darin, could you take a look at my change in
resource_dispatcher_host_unittest.cc? I'm using an additional AtExitManager to
clean up PluginService singleton between tests (it listens to extensions
notifications for later tests, and it's causing problems).

[Aaron Boodman, 2009-11-03 17:56:24]

On 2009/11/03 17:38:42, Paweł Hajdan Jr. wrote:
> Aaron, could you review the latest revision of this patch? It should fix the
> unit_tests crash (and it was a Singleton issue!).

SLGTM

[darin (slow to review), 2009-11-03 18:12:43]

http://codereview.chromium.org/341050/diff/11001/11014
File chrome/browser/renderer_host/resource_dispatcher_host_unittest.cc (right):

http://codereview.chromium.org/341050/diff/11001/11014#newcode39
Line 39: class ShadowingAtExitManager : public base::AtExitManager {
nit: might be better to make AtExitManager be a member variable.  usually best
to avoid inheritance unless you need it.

[Paweł Hajdan Jr., 2009-11-04 14:14:22]

aa: submitted and committed the fixed version

darin (FYI): after a discussion with aa I dropeed the AtExitManager changes
temporarily. I still plan to fix these issues (in other patch).