Remove ServiceIsNullForTesting override for FaviconServiceFactory

Remove ServiceIsNullForTesting override for FaviconServiceFactory

Removes ServiceIsNullForTesting override in FaviconServiceFactory
to enable use of FaviconServiceFactory in tests.

BUG=557415
Committed: https://crrev.com/19acda1ba5c116bbc4cea6c9163d10c65a9ba1c8
Cr-Commit-Position: refs/heads/master@{#361108}

[Jackie Quinn, 2015-11-20 18:40:53]

jyquinn@chromium.org changed reviewers:
+ sdefresne@chromium.org

[sdefresne, 2015-11-21 01:52:03]

TL;DR: BrowserStateKeyedServiceFactory return a valid service by default during
tests unless ServiceIsNULLWhileTesting() is overridden to return false, so I
recommend to instead try to remove the override in FaviconServiceFactory and to
remove all change to IOSChromeLargeIconFactory.

Reference:
https://code.google.com/p/chromium/codesearch#chromium/src/components/keyed_s...

https://codereview.chromium.org/1458703009/diff/20001/ios/chrome/browser/favi...
File ios/chrome/browser/favicon/favicon_service_factory.cc (right):

https://codereview.chromium.org/1458703009/diff/20001/ios/chrome/browser/favi...
ios/chrome/browser/favicon/favicon_service_factory.cc:17:
scoped_ptr<KeyedService> BuildFaviconService(web::BrowserState* context) {
Please put in an anonymous namespace:

namespace ios {
namespace {
scoped_ptr<KeyedService> BuildFaviconService(web::BrowserState* context) {
...

https://codereview.chromium.org/1458703009/diff/20001/ios/chrome/browser/favi...
ios/chrome/browser/favicon/favicon_service_factory.cc:64: bool
FaviconServiceFactory::ServiceIsNULLWhileTesting() const {
Could you investigate whether removing this override works? This would allow the
factory to be used even during testing. I do not think that this service would
cause the test to slow too much, nor cause any failures. If you do remove the
override, then there is no need for GetDefaultFactory() nor the rest of the CL.

https://codereview.chromium.org/1458703009/diff/20001/ios/chrome/browser/favi...
ios/chrome/browser/favicon/favicon_service_factory.cc:68: // Static
s/Static/static/

https://codereview.chromium.org/1458703009/diff/20001/ios/chrome/browser/favi...
File ios/chrome/browser/favicon/favicon_service_factory.h (right):

https://codereview.chromium.org/1458703009/diff/20001/ios/chrome/browser/favi...
ios/chrome/browser/favicon/favicon_service_factory.h:35: // Returns the default
factory used to build IOSChromeLargeIconService. Can be
nit: blank line before the comment (as you did in
ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.h).

s/IOSChromeLargeIconService/FaviconServiceFactory/ (and below)
  ^ a.k.a. copy-n-paste fail ;-)

https://codereview.chromium.org/1458703009/diff/20001/ios/chrome/browser/favi...
File ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.cc
(right):

https://codereview.chromium.org/1458703009/diff/20001/ios/chrome/browser/favi...
ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.cc:18:
scoped_ptr<KeyedService> BuildIOSChromeLargeIconService(
nit: s/BuildIOSChromeLargeIconService/BuildLargeIconService/ (this is a private
name in an anonymous namespace, so no risk of confusion)

https://codereview.chromium.org/1458703009/diff/20001/ios/chrome/browser/favi...
ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.cc:28: }
nit: }  // namespace

https://codereview.chromium.org/1458703009/diff/20001/ios/chrome/browser/favi...
ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.cc:29: //
static
nit: blank line before // static

https://codereview.chromium.org/1458703009/diff/20001/ios/chrome/browser/favi...
File ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.h (right):

https://codereview.chromium.org/1458703009/diff/20001/ios/chrome/browser/favi...
ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.h:12: class
KeyedService;
nit: this is not required as the type is already defined in
components/keyed_service/ios/browser_state_keyed_service_factory.h

https://codereview.chromium.org/1458703009/diff/20001/ios/chrome/browser/favi...
ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.h:53: 
The class does not override ServiceIsNULLWhileTesting() so by default the
factory can be used during testing without registering a custom factory. I think
all changes to
ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.{cc,h} should
be removed (i.e. they make the code more complex for nothing).

[Jackie Quinn, 2015-11-21 02:46:40]

Description was changed from

==========
Add GetDefaultFactory to iOS favicon service factories

Adds GetDefaultFactory to ios::FaviconServiceFactory and
IOSChromeLargeIconServiceFactory so that they can be added with
TestChromeBrowserState::Builder for use in testing.

BUG=557415
==========

to

==========
Remove ServiceIsNullForTesting override for FaviconServiceFactory

Removes ServiceIsNullForTesting override in FaviconServiceFactory
to enable use of FaviconServiceFactory in tests. 

BUG=557415
==========

[Jackie Quinn, 2015-11-21 02:47:08]

It worked!! Thanks a bunch :-)

Much simpler patch uploaded.

[sdefresne, 2015-11-21 08:20:09]

lgtm

[Jackie Quinn, 2015-11-23 15:28:02]

The CQ bit was checked by jyquinn@chromium.org

[commit-bot: I haz the power, 2015-11-23 15:28:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1458703009/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1458703009/40001

[commit-bot: I haz the power, 2015-11-23 15:42:28]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-23 15:43:17]

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