Browser changes for wiring up RequestContentScript API to shared memory

Browser changes for wiring up RequestContentScript API to shared memory

BUG=377978
Committed: https://crrev.com/3ac20004ca071958701de3c000ca197767ae3727
Cr-Commit-Position: refs/heads/master@{#292504}

[Mark Dittmer, 2014-08-21 01:44:15]

Hey Devlin, here are the browser-side changes for connecting the
RequestContentScript API to shared memory. IPC send to renderer is marked as
TODO; it is included in a renderer-side CL to be uploaded presently.

This CL depends on https://codereview.chromium.org/495853002/

[Devlin, 2014-08-21 17:14:09]

I can't really review the declarative stuff (i.e.... most of this) - I'm not
familiar enough with it.  jyasskin@ is the resident expert in that area. :)

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/declarative_content/content_action.h (right):

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/declarative_content/content_action.h:57: const
base::Time& extension_install_time,
indentation.

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/te...
File chrome/browser/extensions/test_extension_system.h (right):

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/te...
chrome/browser/extensions/test_extension_system.h:89: void SetReady() {
why this change?

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/us...
File chrome/browser/extensions/user_script_loader.cc (left):

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/us...
chrome/browser/extensions/user_script_loader.cc:350: clear_scripts_(false),
probably wanna init this still. ;)

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/us...
File chrome/browser/extensions/user_script_loader_unittest.cc (right):

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/us...
chrome/browser/extensions/user_script_loader_unittest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
??

[Devlin, 2014-08-21 17:14:10]

I can't really review the declarative stuff (i.e.... most of this) - I'm not
familiar enough with it.  jyasskin@ is the resident expert in that area. :)

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/declarative_content/content_action.h (right):

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/declarative_content/content_action.h:57: const
base::Time& extension_install_time,
indentation.

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/te...
File chrome/browser/extensions/test_extension_system.h (right):

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/te...
chrome/browser/extensions/test_extension_system.h:89: void SetReady() {
why this change?

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/us...
File chrome/browser/extensions/user_script_loader.cc (left):

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/us...
chrome/browser/extensions/user_script_loader.cc:350: clear_scripts_(false),
probably wanna init this still. ;)

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/us...
File chrome/browser/extensions/user_script_loader_unittest.cc (right):

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/us...
chrome/browser/extensions/user_script_loader_unittest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
??

[Mark Dittmer, 2014-08-21 21:22:22]

Fix poorly merged changes.

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/us...
File chrome/browser/extensions/user_script_loader_unittest.cc (right):

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/us...
chrome/browser/extensions/user_script_loader_unittest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2014/08/21 17:14:09, Devlin wrote:
> ??

Done.

[Mark Dittmer, 2014-08-21 21:25:12]

ditto

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/declarative_content/content_action.h (right):

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/declarative_content/content_action.h:57: const
base::Time& extension_install_time,
On 2014/08/21 17:14:09, Devlin wrote:
> indentation.

Done.

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/te...
File chrome/browser/extensions/test_extension_system.h (right):

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/te...
chrome/browser/extensions/test_extension_system.h:89: void SetReady() {
On 2014/08/21 17:14:09, Devlin wrote:
> why this change?

Done.

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/us...
File chrome/browser/extensions/user_script_loader.cc (left):

https://codereview.chromium.org/493633003/diff/1/chrome/browser/extensions/us...
chrome/browser/extensions/user_script_loader.cc:350: clear_scripts_(false),
On 2014/08/21 17:14:09, Devlin wrote:
> probably wanna init this still. ;)

Done.

[Jeffrey Yasskin, 2014-08-22 01:12:24]

+vabr to double-check the addition of Profile/BrowserContext to the overall
declarative framework. I think it's fine though.

https://codereview.chromium.org/493633003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/declarative_rule.h (right):

https://codereview.chromium.org/493633003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_rule.h:151: static
scoped_ptr<DeclarativeActionSet> Create(Profile* profile,
Can this take a BrowserContext instead of a Profile?

https://codereview.chromium.org/493633003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_rule.h:177: void
Reapply(const std::string& extension_id,
This looks like it duplicates the other Reapply().

https://codereview.chromium.org/493633003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative_content/content_action.cc
(right):

https://codereview.chromium.org/493633003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/declarative_content/content_action.cc:19: #include
"chrome/browser/sessions/session_tab_helper.h"
Double-check which headers you still need.

https://codereview.chromium.org/493633003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/declarative_content/content_action.cc:124:
RequestContentScript(Profile* profile,
Try not to reorder blocks unnecessarily.

https://codereview.chromium.org/493633003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/declarative_content/content_action.cc:263:
ExtensionSystem::Get(profile)->GetDeclarativeUserScriptMasterByExtension(
Can you point to the code that shows that master_ will be destroyed after the
ContentActions?

https://codereview.chromium.org/493633003/diff/40001/chrome/browser/extension...
File
chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc
(right):

https://codereview.chromium.org/493633003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:50:
IN_PROC_BROWSER_TEST_F(RequestContentScriptTest, MissingScripts) {
These look like they just duplicate tests in content_action_unittest.cc. Don't
use a browsertest when you could use a unittest, since browsertests are so much
slower and flakier.

[vabr (Chromium), 2014-08-22 09:48:50]

On 2014/08/22 01:12:24, Jeffrey Yasskin wrote:
> +vabr to double-check the addition of Profile/BrowserContext to the overall
> declarative framework. I think it's fine though.

Thanks for looping me in.

I'm fine with the new use of Profile. This is in //chrome/* after all, and the
surrounding code (rules registries) already know about Profile, so no new
dependencies are being introduced.

Cheers,
Vaclav

[Devlin, 2014-08-22 19:25:45]

https://codereview.chromium.org/493633003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative_content/content_action.cc
(right):

https://codereview.chromium.org/493633003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/declarative_content/content_action.cc:263:
ExtensionSystem::Get(profile)->GetDeclarativeUserScriptMasterByExtension(
On 2014/08/22 01:12:24, Jeffrey Yasskin wrote:
> Can you point to the code that shows that master_ will be destroyed after the
> ContentActions?

Jumping in, since Mark was chatting with me about this. :)

Mark says that the ContentActions are owned by the RulesRegistryService.  If
this is the case, we're guaranteed safe, because the RulesRegistryService is
itself a BrowserActionKeyedAPI.  All BrowserActionKeyedAPIs have a declared
dependency on the ExtensionSystem.

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

[Mark Dittmer, 2014-08-22 19:59:19]

Addressed first round of comments.

https://codereview.chromium.org/493633003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/declarative_rule.h (right):

https://codereview.chromium.org/493633003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_rule.h:151: static
scoped_ptr<DeclarativeActionSet> Create(Profile* profile,
On 2014/08/22 01:12:24, Jeffrey Yasskin wrote:
> Can this take a BrowserContext instead of a Profile?

Yes. Done.

https://codereview.chromium.org/493633003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_rule.h:177: void
Reapply(const std::string& extension_id,
On 2014/08/22 01:12:24, Jeffrey Yasskin wrote:
> This looks like it duplicates the other Reapply().

Yes. Merge fail. Fixed.

https://codereview.chromium.org/493633003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative_content/content_action.cc
(right):

https://codereview.chromium.org/493633003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/declarative_content/content_action.cc:19: #include
"chrome/browser/sessions/session_tab_helper.h"
On 2014/08/22 01:12:24, Jeffrey Yasskin wrote:
> Double-check which headers you still need.

Done.

https://codereview.chromium.org/493633003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/declarative_content/content_action.cc:124:
RequestContentScript(Profile* profile,
On 2014/08/22 01:12:24, Jeffrey Yasskin wrote:
> Try not to reorder blocks unnecessarily.

Done.

https://codereview.chromium.org/493633003/diff/40001/chrome/browser/extension...
File
chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc
(right):

https://codereview.chromium.org/493633003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:50:
IN_PROC_BROWSER_TEST_F(RequestContentScriptTest, MissingScripts) {
On 2014/08/22 01:12:24, Jeffrey Yasskin wrote:
> These look like they just duplicate tests in content_action_unittest.cc. Don't
> use a browsertest when you could use a unittest, since browsertests are so
much
> slower and flakier.

I originally implemented these as unittest, but the code under test needs
non-NULL test extension objects. That's why I moved these from being in unittest
to being browsertest.

[Devlin, 2014-08-22 22:21:30]

On 2014/08/22 19:59:19, Mark Dittmer wrote:
>
https://codereview.chromium.org/493633003/diff/40001/chrome/browser/extension...
>
chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:50:
> IN_PROC_BROWSER_TEST_F(RequestContentScriptTest, MissingScripts) {
> On 2014/08/22 01:12:24, Jeffrey Yasskin wrote:
> > These look like they just duplicate tests in content_action_unittest.cc.
Don't
> > use a browsertest when you could use a unittest, since browsertests are so
> much
> > slower and flakier.
> 
> I originally implemented these as unittest, but the code under test needs
> non-NULL test extension objects. That's why I moved these from being in
unittest
> to being browsertest.

We can have non-NULL extension objects easily.  If you need a full
ExtensionService (I don't think you should, but if you do), subclass
ExtensionServiceTestBase and it'll work.  Otherwise, you should be able to
create a valid extension using ExtensionBuilder and add it to the registry and
most will work.

I think the only potentially tricky part here is that we would need to implement
or mock the GetDeclarativeUserScriptMaster call on ExtensionSystem.  Should just
mean (optionally) overloading the method in TestExtensionSystem.

What were your specific concerns?

[Mark Dittmer, 2014-08-23 16:39:41]

On 2014/08/22 22:21:30, Devlin wrote:
> On 2014/08/22 19:59:19, Mark Dittmer wrote:
> >
>
https://codereview.chromium.org/493633003/diff/40001/chrome/browser/extension...
> >
>
chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:50:
> > IN_PROC_BROWSER_TEST_F(RequestContentScriptTest, MissingScripts) {
> > On 2014/08/22 01:12:24, Jeffrey Yasskin wrote:
> > > These look like they just duplicate tests in content_action_unittest.cc.
> Don't
> > > use a browsertest when you could use a unittest, since browsertests are so
> > much
> > > slower and flakier.
> > 
> > I originally implemented these as unittest, but the code under test needs
> > non-NULL test extension objects. That's why I moved these from being in
> unittest
> > to being browsertest.
> 
> We can have non-NULL extension objects easily.  If you need a full
> ExtensionService (I don't think you should, but if you do), subclass
> ExtensionServiceTestBase and it'll work.  Otherwise, you should be able to
> create a valid extension using ExtensionBuilder and add it to the registry and
> most will work.
> 
> I think the only potentially tricky part here is that we would need to
implement
> or mock the GetDeclarativeUserScriptMaster call on ExtensionSystem.  Should
just
> mean (optionally) overloading the method in TestExtensionSystem.
> 
> What were your specific concerns?

First, let me explain the code duplication across unittest and browsertest. When
I had trouble getting things working without a profile/browser_context +
extension, I moved my code from unittest to a new browsertest file. Subsequent
branch juggling + rebasing pushed both versions of the tests into the CL by
mistake.

I was able to refactor the tests as unittests, but it was a larger refactor than
I expected. Based on the following observations:

(1) Generalizing a "CreateForTest(...)" factory for all ContentAction subclasses
would require some crazy dynamism because the data injected instead of
browser_context would vary from subclass to subclass (in the case of
RequestContentScript, a DeclarativeUserScriptMaster would be injected, rather
than looked up via the ExtensionSystem).
(2) DeclarativeUserScriptMaster is hard to make Fake or Mock because it has no
virtual interfaces.
(3) Using template<typename Fake/Mock/RealUserScriptMasterType> in every place
we have a DeclarativeUserScriptMaster seems verbose/overkill.

... I made the following changes:

(1a) Move RequestContentScript to its header.
(1b) Add a CreateForTest(...) to RequestContentScript.
(1c) To facilitate consistency between ContentAction::Create(...) and
RequestContentScript::CreateForTest(...), refactor the former to invoke public
helper functions for procedures the two have in common.
(2) Implement FakeDeclarativeUserScriptMaster for RequestContentScript tests.
(3) To facilitate 2 above, split DeclarativeUserScriptMaster into an interface +
an Impl class.

I hope this is an agreeable solution. It does eliminate the need for
browsertests, which is great. My current implementations augment
content_action.h, content_action_unittest.cc, and
declarative_user_script_master.h with the additional classes. Let me know if any
of these new classes deserve their own files.

One thing that's a little weird is that one of ContentAction's subclasses is
contained in its cc file, whereas another is split across the header and cc
file. This may be a bit misleading to any developer that assumes all subclasses
live in one file or the other.

[Mark Dittmer, 2014-08-25 22:03:39]

Missed a change to gypi file + presubmit warning (non-explicit single-param
ctor). Fixed in latest patch.

[Devlin, 2014-08-25 22:44:43]

This all seems... excessive.

Maybe I'm missing something, but it seemed like in the previous patch the only
thing really being tested was the creation of the action, and the only thing
that needed to be done for that was to have some sort of
DeclarativeUserScriptMaster.  Couldn't we just make TestExtensionSystem return a
real DeclarativeUserScriptMaster?  It's a very light class, and doesn't really
do anything if there are no renderers around...

What's the need for all the added complexity?

[Mark Dittmer, 2014-08-26 12:18:15]

On 2014/08/25 22:44:43, Devlin wrote:
> This all seems... excessive.
> 
> Maybe I'm missing something, but it seemed like in the previous patch the only
> thing really being tested was the creation of the action, and the only thing
> that needed to be done for that was to have some sort of
> DeclarativeUserScriptMaster.  Couldn't we just make TestExtensionSystem return
a
> real DeclarativeUserScriptMaster?  It's a very light class, and doesn't really
> do anything if there are no renderers around...
> 
> What's the need for all the added complexity?

The complexity is to avoid using both a real declarative master and a real
loader. Taken together, their constructors both access a global
ExtensionRegistry and register a total of three observers. As far as I can tell,
our options are:

(1) Leave the code as it is now.
(2) Inject an ExtensionRegistry to declarative master, and when it's NULL don't
register an observer; and
(2a) Create a mock or fake loader for the master; or
(2b) Change the loader to follow the same inject-and-maybe-skip-observers logic
mentioned in (2).

We end up either hacking production object construction code to do something
special when being used for tests, or doing some degree of fake/mock objects.
Since the code under test is the content action code, and not the declarative
master code, I feel the cleanest thing to do is mock/fake the direct dependency
of the code under test: the declarative master class.

[Devlin, 2014-08-26 15:23:32]

On 2014/08/26 12:18:15, Mark Dittmer wrote:
> On 2014/08/25 22:44:43, Devlin wrote:
> > This all seems... excessive.
> > 
> > Maybe I'm missing something, but it seemed like in the previous patch the
only
> > thing really being tested was the creation of the action, and the only thing
> > that needed to be done for that was to have some sort of
> > DeclarativeUserScriptMaster.  Couldn't we just make TestExtensionSystem
return
> a
> > real DeclarativeUserScriptMaster?  It's a very light class, and doesn't
really
> > do anything if there are no renderers around...
> > 
> > What's the need for all the added complexity?
> 
> The complexity is to avoid using both a real declarative master and a real
> loader. Taken together, their constructors both access a global
> ExtensionRegistry and register a total of three observers. As far as I can
tell,
> our options are:
> 
> (1) Leave the code as it is now.
> (2) Inject an ExtensionRegistry to declarative master, and when it's NULL
don't
> register an observer; and
> (2a) Create a mock or fake loader for the master; or
> (2b) Change the loader to follow the same inject-and-maybe-skip-observers
logic
> mentioned in (2).
> 
> We end up either hacking production object construction code to do something
> special when being used for tests, or doing some degree of fake/mock objects.
> Since the code under test is the content action code, and not the declarative
> master code, I feel the cleanest thing to do is mock/fake the direct
dependency
> of the code under test: the declarative master class.

Still not sure I agree.

We can have ExtensionRegistries in unit tests with no problem, and having
observers of those registries is similarly fine.  The fact that the loader
registers itself for notifications is similarly not problematic, AFAIK.  Could
you be more specific about what the concrete and immediate problems are?  E.g.,
why would a test using a real DeclarativeScriptMaster fail/crash/not work?

[Jeffrey Yasskin, 2014-08-26 15:37:34]

On Tue, Aug 26, 2014 at 8:23 AM,  <rdevlin.cronin@chromium.org> wrote:
> On 2014/08/26 12:18:15, Mark Dittmer wrote:
>>
>> On 2014/08/25 22:44:43, Devlin wrote:
>> > This all seems... excessive.
>> >
>> > Maybe I'm missing something, but it seemed like in the previous patch
>> > the
>
> only
>>
>> > thing really being tested was the creation of the action, and the only
>> > thing
>> > that needed to be done for that was to have some sort of
>> > DeclarativeUserScriptMaster.  Couldn't we just make TestExtensionSystem
>
> return
>>
>> a
>> > real DeclarativeUserScriptMaster?  It's a very light class, and doesn't
>
> really
>>
>> > do anything if there are no renderers around...
>> >
>> > What's the need for all the added complexity?
>
>
>> The complexity is to avoid using both a real declarative master and a real
>> loader. Taken together, their constructors both access a global
>> ExtensionRegistry and register a total of three observers. As far as I can
>
> tell,
>>
>> our options are:
>
>
>> (1) Leave the code as it is now.
>> (2) Inject an ExtensionRegistry to declarative master, and when it's NULL
>
> don't
>>
>> register an observer; and
>> (2a) Create a mock or fake loader for the master; or
>> (2b) Change the loader to follow the same inject-and-maybe-skip-observers
>
> logic
>>
>> mentioned in (2).
>
>
>> We end up either hacking production object construction code to do
>> something
>> special when being used for tests, or doing some degree of fake/mock
>> objects.
>> Since the code under test is the content action code, and not the
>> declarative
>> master code, I feel the cleanest thing to do is mock/fake the direct
>
> dependency
>>
>> of the code under test: the declarative master class.
>
>
> Still not sure I agree.
>
> We can have ExtensionRegistries in unit tests with no problem, and having
> observers of those registries is similarly fine.  The fact that the loader
> registers itself for notifications is similarly not problematic, AFAIK.
> Could
> you be more specific about what the concrete and immediate problems are?
> E.g.,
> why would a test using a real DeclarativeScriptMaster fail/crash/not work?

FWIW, +1 to this.

> https://codereview.chromium.org/493633003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark Dittmer, 2014-08-27 20:52:33]

Got unittests working with real DeclarativeUserScriptMaster. I am concerned
about the required
TestExtensionSystem::GetDeclarativeUserScriptMasterByExtension. It is identical
to ExtensionSystemImpl::Shared::GetDeclarativeUserScriptMasterByExtension, but
this is not enforced. As such, if the latter diverges, tests using the former
may begin to test the wrong thing.

[Devlin, 2014-08-27 21:21:21]

On 2014/08/27 20:52:33, Mark Dittmer wrote:
> Got unittests working with real DeclarativeUserScriptMaster. I am concerned
> about the required
> TestExtensionSystem::GetDeclarativeUserScriptMasterByExtension. It is
identical
> to ExtensionSystemImpl::Shared::GetDeclarativeUserScriptMasterByExtension, but
> this is not enforced. As such, if the latter diverges, tests using the former
> may begin to test the wrong thing.

I wouldn't worry about it too much, as we're not testing
ExtensionSystem::GetDeclarativeUserScriptMasterByExtension().  But that's just
me.

[Devlin, 2014-08-27 21:22:06]

(Only reviewing unittest structure)

https://codereview.chromium.org/493633003/diff/140001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc
(right):

https://codereview.chromium.org/493633003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:18:
#include "extensions/browser/extension_registry.h"
don't need.

https://codereview.chromium.org/493633003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:53:
profile_.reset(TestingProfile::Builder().Build().release());
Two things:
1. These should almost certainly not go here.  Instead, look at
testing::Test::SetUp().
2. The profile you're referencing is a member of ExtensionServiceTestBase (yes,
there's a TODO to make it private so this can't happen).  This profile will
actually be reset as part of ExtensionServiceTestBase's initing.  So it's really
not doing anything.

The only initialization you should need to do is:
a) maybe call InitializeEmptyExtensionService() from within your test.
b) maybe call TestExtensionSystem::SetReady().

[Mark Dittmer, 2014-08-28 12:22:17]

Cleaned up test initialization. It would be nice to get this into CQ today,
since I finish tomorrow. Of course, only if it LGTY.

https://codereview.chromium.org/493633003/diff/140001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc
(right):

https://codereview.chromium.org/493633003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:53:
profile_.reset(TestingProfile::Builder().Build().release());
On 2014/08/27 21:22:05, Devlin wrote:
> Two things:
> 1. These should almost certainly not go here.  Instead, look at
> testing::Test::SetUp().
> 2. The profile you're referencing is a member of ExtensionServiceTestBase
(yes,
> there's a TODO to make it private so this can't happen).  This profile will
> actually be reset as part of ExtensionServiceTestBase's initing.  So it's
really
> not doing anything.
> 
> The only initialization you should need to do is:
> a) maybe call InitializeEmptyExtensionService() from within your test.
> b) maybe call TestExtensionSystem::SetReady().

Done. I'm not sure what you meant by "within your test". I put both calls in
SetUp() because the former creates a profile and the latter is called using a
getter that needs a profile.

[Devlin, 2014-08-28 17:58:10]

https://codereview.chromium.org/493633003/diff/140001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc
(right):

https://codereview.chromium.org/493633003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:53:
profile_.reset(TestingProfile::Builder().Build().release());
On 2014/08/28 12:22:17, Mark Dittmer wrote:
> On 2014/08/27 21:22:05, Devlin wrote:
> > Two things:
> > 1. These should almost certainly not go here.  Instead, look at
> > testing::Test::SetUp().
> > 2. The profile you're referencing is a member of ExtensionServiceTestBase
> (yes,
> > there's a TODO to make it private so this can't happen).  This profile will
> > actually be reset as part of ExtensionServiceTestBase's initing.  So it's
> really
> > not doing anything.
> > 
> > The only initialization you should need to do is:
> > a) maybe call InitializeEmptyExtensionService() from within your test.
> > b) maybe call TestExtensionSystem::SetReady().
> 
> Done. I'm not sure what you meant by "within your test". I put both calls in
> SetUp() because the former creates a profile and the latter is called using a
> getter that needs a profile.

Sometimes, there's a problem with calling InitializeEmptyExtensionService() from
SetUp() (haven't had a chance to investigate why), so we typically call if from
within the test body itself, e.g.
TEST(Foo, Bar) {
  InitializeEmptyExtensionService()...
}

https://codereview.chromium.org/493633003/diff/160001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc
(right):

https://codereview.chromium.org/493633003/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:56:
void RequestContentScriptTest::SetUp() {
You should always call the superclass's SetUp() method, too.

void RequestContentScriptTest::SetUp() {
  ExtensionServiceTestBase::SetUp();
  <own set up>
}

[Mark Dittmer, 2014-08-28 19:57:08]

Fix/nix unit tests SetUp()

https://codereview.chromium.org/493633003/diff/140001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc
(right):

https://codereview.chromium.org/493633003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:18:
#include "extensions/browser/extension_registry.h"
On 2014/08/27 21:22:05, Devlin wrote:
> don't need.

Done.

https://codereview.chromium.org/493633003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:53:
profile_.reset(TestingProfile::Builder().Build().release());
On 2014/08/28 17:58:10, Devlin wrote:
> On 2014/08/28 12:22:17, Mark Dittmer wrote:
> > On 2014/08/27 21:22:05, Devlin wrote:
> > > Two things:
> > > 1. These should almost certainly not go here.  Instead, look at
> > > testing::Test::SetUp().
> > > 2. The profile you're referencing is a member of ExtensionServiceTestBase
> > (yes,
> > > there's a TODO to make it private so this can't happen).  This profile
will
> > > actually be reset as part of ExtensionServiceTestBase's initing.  So it's
> > really
> > > not doing anything.
> > > 
> > > The only initialization you should need to do is:
> > > a) maybe call InitializeEmptyExtensionService() from within your test.
> > > b) maybe call TestExtensionSystem::SetReady().
> > 
> > Done. I'm not sure what you meant by "within your test". I put both calls in
> > SetUp() because the former creates a profile and the latter is called using
a
> > getter that needs a profile.
> 
> Sometimes, there's a problem with calling InitializeEmptyExtensionService()
from
> SetUp() (haven't had a chance to investigate why), so we typically call if
from
> within the test body itself, e.g.
> TEST(Foo, Bar) {
>   InitializeEmptyExtensionService()...
> }

Very unfortunate. The subsequent lines depend on it. Refactored.

https://codereview.chromium.org/493633003/diff/160001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc
(right):

https://codereview.chromium.org/493633003/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:56:
void RequestContentScriptTest::SetUp() {
On 2014/08/28 17:58:10, Devlin wrote:
> You should always call the superclass's SetUp() method, too.
> 
> void RequestContentScriptTest::SetUp() {
>   ExtensionServiceTestBase::SetUp();
>   <own set up>
> }

I'll keep that in mind. This has been replaced with non-override "Init()" and is
invoked by every test manually (due to issue with
InitializeEmptyExtensionService() you mentioned previously).

[Jeffrey Yasskin, 2014-08-28 21:38:48]

lgtm

https://codereview.chromium.org/493633003/diff/180001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc
(right):

https://codereview.chromium.org/493633003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:56:
RequestContentScriptTest::RequestContentScriptTest()
Inside a test, it's generally good to define methods inline.

[Devlin, 2014-08-28 21:47:10]

If Jeffrey's happy, I'm happy. LGTM.

https://codereview.chromium.org/493633003/diff/180001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc
(right):

https://codereview.chromium.org/493633003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:15:
#include "chrome/test/base/testing_profile.h"
don't need

https://codereview.chromium.org/493633003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:43:
// TODO(rdevlin.cronin): This should be SetUp(), but an issues with invoking
nit: not "an issues"

https://codereview.chromium.org/493633003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:56:
RequestContentScriptTest::RequestContentScriptTest()
On 2014/08/28 21:38:47, Jeffrey Yasskin wrote:
> Inside a test, it's generally good to define methods inline.

He probably got this from me.  I've heard it a bit both ways, and there was a
very passionate discussion here:
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/inline...

which (from a brief skim) didn't really seem to come to a decided conclusion.

I generally like to define longer methods out-of-line in tests, but for
constructors and the simple init, it does seem superfluous.  And, in any event,
OWNERS rules. :)

[Mark Dittmer, 2014-08-28 21:59:47]

Fixed last nit.

https://codereview.chromium.org/493633003/diff/180001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc
(right):

https://codereview.chromium.org/493633003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:56:
RequestContentScriptTest::RequestContentScriptTest()
On 2014/08/28 21:38:47, Jeffrey Yasskin wrote:
> Inside a test, it's generally good to define methods inline.

Done.

[Mark Dittmer, 2014-08-28 21:59:55]

The CQ bit was checked by markdittmer@chromium.org

[commit-bot: I haz the power, 2014-08-28 22:00:36]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/markdittmer@chromium.org/493633003/200001

[commit-bot: I haz the power, 2014-08-28 23:43:15]

Committed patchset #11 (id:200001) as 6bec27acdc4867c87ebb37a3460b2fbe96831cc1

[commit-bot: I haz the power, 2014-09-10 03:03:46]

Patchset 11 (id:??) landed as
https://crrev.com/3ac20004ca071958701de3c000ca197767ae3727
Cr-Commit-Position: refs/heads/master@{#292504}