Allow overriding WebUIFactory when test flag is set.

Allow overriding WebUIFactory when test flag is set.

In order to allow mocking function calls as soon as possible (before any javascript is
run), we create a factory capable of registering overrides per-host.

BUG=82437
R=estade@chromium.org
TEST=browser_tests --gtest_filter=TestChromeWebUIFactoryTest.*


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

[Sheridan Rawlins, 2011-05-24 05:57:05]

Evan,

You mentioned refactoring the ChomeWebUIFactory recently.  The hope of this new
class is to provide an override mechanism which is only used for test cases.

[Evan Stade, 2011-05-24 17:25:59]

just nits

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chrome_web_ui_factory.cc (right):

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chrome_web_ui_factory.cc:224: struct
PossibleTestSingletonTraits : public DefaultSingletonTraits<Type> {
explicitly state privacy level

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
File chrome/browser/ui/webui/test_chrome_web_ui_factory.cc (right):

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
chrome/browser/ui/webui/test_chrome_web_ui_factory.cc:20: void
wrap the args rather than the return type

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
chrome/browser/ui/webui/test_chrome_web_ui_factory.cc:23: DCHECK_EQ(size_t(0),
GetInstance()->factory_overrides_.count(host));
0U rather than size_t(0)

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
chrome/browser/ui/webui/test_chrome_web_ui_factory.cc:58: if (found !=
factory_overrides_.end()) {
no curlies

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
File chrome/browser/ui/webui/test_chrome_web_ui_factory.h (right):

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
chrome/browser/ui/webui/test_chrome_web_ui_factory.h:66:
DISALLOW_IMPLICIT_CONSTRUCTORS(TestChromeWebUIFactory);
I think you should use DISALLOW_COPY_AND_ASSIGN and explicitly declare the
default constructor above with the destructor

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
File chrome/browser/ui/webui/test_chrome_web_ui_factory_browsertest.cc (right):

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
chrome/browser/ui/webui/test_chrome_web_ui_factory_browsertest.cc:46:
GURL(kChromeTestChromeWebUIFactory).host());
is there a reason to do this setup and teardown here rather than in the test
itself (if you don't plan to write any more tests for TestChromeWebUIFactory)

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
chrome/browser/ui/webui/test_chrome_web_ui_factory_browsertest.cc:57:
.WillOnce(ReturnNewWebUI());
. goes on EOL

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
chrome/browser/ui/webui/test_chrome_web_ui_factory_browsertest.cc:61: } 
//namespace
space after //

[Lei Zhang (Do not use), 2011-05-24 18:00:06]

some nits below: (whoops, just realized I was using the wrong acct)

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chrome_web_ui_factory.cc (right):

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chrome_web_ui_factory.cc:224: struct
PossibleTestSingletonTraits : public DefaultSingletonTraits<Type> {
Do you need this template? Why can't you just do the following in GetInstance()
?

if (CommandLine::ForCurrentProcess()->HasSwitch(some_flag))
  return Singleton<TestChromeWebUIFactory>::get();
else
  return Singleton<ChromeWebUIFactory>::get();

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chrome_web_ui_factory.h (right):

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chrome_web_ui_factory.h:39:
DISALLOW_IMPLICIT_CONSTRUCTORS(ChromeWebUIFactory);
nit: this should be last.

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
File chrome/browser/ui/webui/test_chrome_web_ui_factory.h (right):

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
chrome/browser/ui/webui/test_chrome_web_ui_factory.h:21: // flag is passed. It
provides a registry to override |CreateWebUIForURL| by
nit: |CreateWebUIForURL| should be just CreateWebUIForURL().

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
File chrome/browser/ui/webui/test_chrome_web_ui_factory_browsertest.cc (right):

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
chrome/browser/ui/webui/test_chrome_web_ui_factory_browsertest.cc:61: } 
//namespace
On 2011/05/24 17:25:59, Evan Stade wrote:
> space after //

No need to put the actual tests inside an anonymous namespace.

[Sheridan Rawlins, 2011-05-24 20:17:21]

I addressed some nits from review and replied to others.  Please consider the
defense, and let me know if there's still any adjustments to make.

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chrome_web_ui_factory.cc (right):

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chrome_web_ui_factory.cc:224: struct
PossibleTestSingletonTraits : public DefaultSingletonTraits<Type> {
This doesn't play nicely with the privacy/friendship example usage in
base/memory/singleton.h (get is only allowed in the GetIntance of the particular
type.

On 2011/05/24 18:00:06, thestig wrote:
> Do you need this template? Why can't you just do the following in
GetInstance()
> ?
> 
> if (CommandLine::ForCurrentProcess()->HasSwitch(some_flag))
>   return Singleton<TestChromeWebUIFactory>::get();
> else
>   return Singleton<ChromeWebUIFactory>::get();

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chrome_web_ui_factory.cc:224: struct
PossibleTestSingletonTraits : public DefaultSingletonTraits<Type> {
On 2011/05/24 17:25:59, Evan Stade wrote:
> explicitly state privacy level

As a derived struct of DefaultSingletonTraits, which is a struct, I'll leave
this a struct too, as we discussed (struct's default privacy level is public).

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chrome_web_ui_factory.h (right):

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chrome_web_ui_factory.h:39:
DISALLOW_IMPLICIT_CONSTRUCTORS(ChromeWebUIFactory);
Ok, based upon Evan's comments and the example, I changed to
DISALLOW_COPY_AND_ASSIGN, leaving the constructor in the protected section so
that the subclass could instantiate itself.

On 2011/05/24 18:00:06, thestig wrote:
> nit: this should be last.

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
File chrome/browser/ui/webui/test_chrome_web_ui_factory.cc (right):

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
chrome/browser/ui/webui/test_chrome_web_ui_factory.cc:20: void
On 2011/05/24 17:25:59, Evan Stade wrote:
> wrap the args rather than the return type

Done.

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
chrome/browser/ui/webui/test_chrome_web_ui_factory.cc:23: DCHECK_EQ(size_t(0),
GetInstance()->factory_overrides_.count(host));
On 2011/05/24 17:25:59, Evan Stade wrote:
> 0U rather than size_t(0)

Done.

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
chrome/browser/ui/webui/test_chrome_web_ui_factory.cc:58: if (found !=
factory_overrides_.end()) {
On 2011/05/24 17:25:59, Evan Stade wrote:
> no curlies

Done.

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
File chrome/browser/ui/webui/test_chrome_web_ui_factory.h (right):

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
chrome/browser/ui/webui/test_chrome_web_ui_factory.h:21: // flag is passed. It
provides a registry to override |CreateWebUIForURL| by
On 2011/05/24 18:00:06, thestig wrote:
> nit: |CreateWebUIForURL| should be just CreateWebUIForURL().

Done.

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
chrome/browser/ui/webui/test_chrome_web_ui_factory.h:66:
DISALLOW_IMPLICIT_CONSTRUCTORS(TestChromeWebUIFactory);
On 2011/05/24 17:25:59, Evan Stade wrote:
> I think you should use DISALLOW_COPY_AND_ASSIGN and explicitly declare the
> default constructor above with the destructor

Done.

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
File chrome/browser/ui/webui/test_chrome_web_ui_factory_browsertest.cc (right):

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
chrome/browser/ui/webui/test_chrome_web_ui_factory_browsertest.cc:46:
GURL(kChromeTestChromeWebUIFactory).host());
In fact, putting this here uncovered a bug.  I wanted to make sure that I was
only called once, and when I had GURL as a global constant, it was constructed
before the url_parsing system was initialized and was registering a host of "". 
If I move this into the test, then I don't believe the initial browse to
about:blank goes through the test factory.

On 2011/05/24 17:25:59, Evan Stade wrote:
> is there a reason to do this setup and teardown here rather than in the test
> itself (if you don't plan to write any more tests for TestChromeWebUIFactory)

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
chrome/browser/ui/webui/test_chrome_web_ui_factory_browsertest.cc:57:
.WillOnce(ReturnNewWebUI());
I'd prefer to keep in the style of gmock examples, which start each expecataion
line with a dot (http://code.google.com/p/googlemock/wiki/CheatSheet)

On 2011/05/24 17:25:59, Evan Stade wrote:
> . goes on EOL

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
chrome/browser/ui/webui/test_chrome_web_ui_factory_browsertest.cc:61: } 
//namespace
On 2011/05/24 17:25:59, Evan Stade wrote:
> space after //

Done.

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
chrome/browser/ui/webui/test_chrome_web_ui_factory_browsertest.cc:61: } 
//namespace
On 2011/05/24 18:00:06, thestig wrote:
> On 2011/05/24 17:25:59, Evan Stade wrote:
> > space after //
> 
> No need to put the actual tests inside an anonymous namespace.

Done.

[Sheridan Rawlins, 2011-05-25 18:29:42]

Added comment.

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chrome_web_ui_factory.cc (right):

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chrome_web_ui_factory.cc:224: struct
PossibleTestSingletonTraits : public DefaultSingletonTraits<Type> {
As discussed in email thread, my main argument for doing it here in traits
instead of in GetInstance() is that the if condition will be done exactly once
(when the singleton doesn't exist) rather than with every call to GetInstance().
 While the time savings may be minimal, it is conceptually what I want.  I'll
add a comment.

On 2011/05/24 18:00:06, thestig wrote:
> Do you need this template? Why can't you just do the following in
GetInstance()
> ?
> 
> if (CommandLine::ForCurrentProcess()->HasSwitch(some_flag))
>   return Singleton<TestChromeWebUIFactory>::get();
> else
>   return Singleton<ChromeWebUIFactory>::get();

[Evan Stade, 2011-05-25 18:49:03]

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
File chrome/browser/ui/webui/test_chrome_web_ui_factory_browsertest.cc (right):

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
chrome/browser/ui/webui/test_chrome_web_ui_factory_browsertest.cc:46:
GURL(kChromeTestChromeWebUIFactory).host());
no, I meant why is everything within SetUp and TearDown not just inside the test
itself?

re: GURL as global constant: that's why non-PODs should never be global.

[Sheridan Rawlins, 2011-05-25 21:02:55]

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
File chrome/browser/ui/webui/test_chrome_web_ui_factory_browsertest.cc (right):

http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
chrome/browser/ui/webui/test_chrome_web_ui_factory_browsertest.cc:46:
GURL(kChromeTestChromeWebUIFactory).host());
If I move the add/remove to the method, then I'm not registered early enough to
catch cases where I'm called unexpectedly.

To prove this empirically, I added AddFactoryOverride("", &mock_provider_) both
here and in the test, and only caught the spurious call to the mock when the
code was in the SetUpInProcessBrowserTestFixture().

On 2011/05/25 18:49:04, Evan Stade wrote:
> no, I meant why is everything within SetUp and TearDown not just inside the
test
> itself?
> 
> re: GURL as global constant: that's why non-PODs should never be global.

[Evan Stade, 2011-05-25 21:14:13]

lgtm

-- Evan Stade



On Wed, May 25, 2011 at 2:02 PM,  <scr@chromium.org> wrote:
>
>
http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
> File chrome/browser/ui/webui/test_chrome_web_ui_factory_browsertest.cc
> (right):
>
>
http://codereview.chromium.org/6992019/diff/5001/chrome/browser/ui/webui/test...
> chrome/browser/ui/webui/test_chrome_web_ui_factory_browsertest.cc:46:
> GURL(kChromeTestChromeWebUIFactory).host());
> If I move the add/remove to the method, then I'm not registered early
> enough to catch cases where I'm called unexpectedly.
>
> To prove this empirically, I added AddFactoryOverride("",
> &mock_provider_) both here and in the test, and only caught the spurious
> call to the mock when the code was in the
> SetUpInProcessBrowserTestFixture().
>
> On 2011/05/25 18:49:04, Evan Stade wrote:
>>
>> no, I meant why is everything within SetUp and TearDown not just
>
> inside the test
>>
>> itself?
>
>> re: GURL as global constant: that's why non-PODs should never be
>
> global.
>
> http://codereview.chromium.org/6992019/
>

[Lei Zhang, 2011-05-25 21:21:18]

LGTM

http://codereview.chromium.org/6992019/diff/19/chrome/browser/ui/webui/test_c...
File chrome/browser/ui/webui/test_chrome_web_ui_factory.cc (right):

http://codereview.chromium.org/6992019/diff/19/chrome/browser/ui/webui/test_c...
chrome/browser/ui/webui/test_chrome_web_ui_factory.cc:7: #include
"chrome/browser/ui/webui/chrome_web_ui_factory.h"
nit: You don't need to include this and web_ui.h again. You already include them
in test_chrome_web_ui_factory.h.

[commit-bot: I haz the power, 2011-05-26 01:43:03]

Change committed as 86764