Extract a MockExtensionSystem to be used with TestExtensionsBrowserClient.

Extract a MockExtensionSystem to be used with TestExtensionsBrowserClient.

The default implementation provides no subservices. Its purpose is to fulfill a dependency (in unit tests)
when other BrowserContextKeyedServices depend on an ExtensionSystem. Tests that actually need subservices
can override the necessary methods to provide them.

Also, this defines a MockExtensionSystemFactory template for ease of use with MockExtensionSystem subclasses.

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

[Yoyo Zhou, 2014-08-12 02:00:03]

(Note: By the way, I have successfully used this to move ExtensionApiUnittest
into extensions_unittests; I'll send that out tomorrow after cleaning it up.)

[James Cook, 2014-08-12 15:46:27]

LGTM!  I really like this.  A couple nits.

https://codereview.chromium.org/460203002/diff/20001/extensions/browser/mock_...
File extensions/browser/mock_extension_system.cc (right):

https://codereview.chromium.org/460203002/diff/20001/extensions/browser/mock_...
extensions/browser/mock_extension_system.cc:7: #include
"content/public/browser/browser_context.h"
Do you need this header?

https://codereview.chromium.org/460203002/diff/20001/extensions/browser/mock_...
extensions/browser/mock_extension_system.cc:8: #include
"extensions/common/extension_set.h"
Aside: This is why I don't like CreateFoo() methods that return scoped_ptr<>.
While I understand that you need the destructor to create a null-ish
scoped_ptr<Foo>() it's not trivially obvious.

https://codereview.chromium.org/460203002/diff/20001/extensions/browser/mock_...
File extensions/browser/mock_extension_system.h (right):

https://codereview.chromium.org/460203002/diff/20001/extensions/browser/mock_...
extensions/browser/mock_extension_system.h:19: // To use this, use
TestExtensionsBrowserClient::set_extension_system_factory
nit: either blank line above or wrap together

https://codereview.chromium.org/460203002/diff/20001/extensions/browser/mock_...
extensions/browser/mock_extension_system.h:45: virtual const OneShotEvent&
ready() const OVERRIDE;
Aside, not for this CL: I would like this method to return a OneShotEvent*. Then
test classes like this one would not need to have a OneShotEvent member, a
header include could be avoided, and it would be simple to return NULL. It's
also semantically weird that you can add data to a const OneShotEvent&, but
that's what you have to do to register a callback via Post().

WDYT?

https://codereview.chromium.org/460203002/diff/20001/extensions/browser/mock_...
extensions/browser/mock_extension_system.h:53: };
DISALLOW_COPY_AND_ASSIGN

[Yoyo Zhou, 2014-08-12 22:05:48]

https://codereview.chromium.org/460203002/diff/20001/extensions/browser/mock_...
File extensions/browser/mock_extension_system.cc (right):

https://codereview.chromium.org/460203002/diff/20001/extensions/browser/mock_...
extensions/browser/mock_extension_system.cc:7: #include
"content/public/browser/browser_context.h"
On 2014/08/12 15:46:27, James Cook wrote:
> Do you need this header?

Good call.

https://codereview.chromium.org/460203002/diff/20001/extensions/browser/mock_...
extensions/browser/mock_extension_system.cc:8: #include
"extensions/common/extension_set.h"
On 2014/08/12 15:46:27, James Cook wrote:
> Aside: This is why I don't like CreateFoo() methods that return scoped_ptr<>.
> While I understand that you need the destructor to create a null-ish
> scoped_ptr<Foo>() it's not trivially obvious.

It is unfortunate.

https://codereview.chromium.org/460203002/diff/20001/extensions/browser/mock_...
File extensions/browser/mock_extension_system.h (right):

https://codereview.chromium.org/460203002/diff/20001/extensions/browser/mock_...
extensions/browser/mock_extension_system.h:19: // To use this, use
TestExtensionsBrowserClient::set_extension_system_factory
On 2014/08/12 15:46:27, James Cook wrote:
> nit: either blank line above or wrap together
Wrapped.

https://codereview.chromium.org/460203002/diff/20001/extensions/browser/mock_...
extensions/browser/mock_extension_system.h:45: virtual const OneShotEvent&
ready() const OVERRIDE;
On 2014/08/12 15:46:27, James Cook wrote:
> Aside, not for this CL: I would like this method to return a OneShotEvent*.
Then
> test classes like this one would not need to have a OneShotEvent member, a
> header include could be avoided, and it would be simple to return NULL. It's
> also semantically weird that you can add data to a const OneShotEvent&, but
> that's what you have to do to register a callback via Post().
> 
> WDYT?

Yeah, I agree.

https://codereview.chromium.org/460203002/diff/20001/extensions/browser/mock_...
extensions/browser/mock_extension_system.h:53: };
On 2014/08/12 15:46:27, James Cook wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[Yoyo Zhou, 2014-08-12 22:05:52]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-08-12 22:10:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoz@chromium.org/460203002/40001

[commit-bot: I haz the power, 2014-08-13 00:34:14]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Yoyo Zhou, 2014-08-13 00:37:48]

The CQ bit was unchecked by yoz@chromium.org

[Yoyo Zhou, 2014-08-13 00:45:39]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-08-13 01:07:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoz@chromium.org/460203002/60001

[commit-bot: I haz the power, 2014-08-13 09:46:25]

Change committed as 289235