[Fallback Icons] Refactor FallbackIconService to be a BrowserContext-level singleton

components/favicon/core/browser/fallback_icon_client.h

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_FAVICON_CORE_BROWSER_FALLBACK_ICON_CLIENT_H_
+#define COMPONENTS_FAVICON_CORE_BROWSER_FALLBACK_ICON_CLIENT_H_
+
+#include <string>
+#include <vector>
+
+#include "components/keyed_service/core/keyed_service.h"
+
+namespace favicon_base {
+class FallbackIconService;
+}  // namespace favicon_base
+
+// This class abstracts operations that depend on the embedder's environment,
+// e.g. Chrome.
+class FallbackIconClient : public KeyedService {
+ public:
+  ~FallbackIconClient() override {}
+
+  // Returns an instance of FallbackIconService.
+  virtual favicon_base::FallbackIconService* GetFallbackIconService() = 0;

[sdefresne, 2015/03/16 09:11:29]

Don't provide virtual method on the ${Feature}Client to access the
${Feature}Service, this is an anti-pattern I'd like to see disappear.

The code accessing the Client should either be passed a pointer to the Service
if it also need the service, or if accessing the Client through a factory, it
can just use the Service factory instead.

The reason for opposing to this pattern is that the Service already has a
dependency on the Client. So if the Client has a dependency on the Service we
have a case of circular dependency, which does not work.

More generally the Client is supposed to be an implementation detail of the
Service and not accessed outside of the Service.

[huangs, 2015/03/16 22:06:11]

Ah oops. I thought that's the endorsed pattern, hence the change. Done.

+
+  // Returns a list of font names for fallback icon rendering.
+  virtual const std::vector<std::string>& GetFontNameList() = 0;
+};
+
+#endif  // COMPONENTS_FAVICON_CORE_BROWSER_FALLBACK_ICON_CLIENT_H_