Extend LargeIconService to fetch missing favicons from a Google server

components/favicon/core/BUILD.gn

 # Copyright 2014 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.
 
 static_library("core") {
   sources = [
     "fallback_url_util.cc",
     "fallback_url_util.h",
     "favicon_client.h",
     "favicon_driver.cc",
     "favicon_driver.h",
     "favicon_driver_impl.cc",
     "favicon_driver_impl.h",
     "favicon_driver_observer.h",
     "favicon_handler.cc",
     "favicon_handler.h",
     "favicon_service.cc",
     "favicon_service.h",
     "favicon_service_impl.cc",
     "favicon_service_impl.h",
     "favicon_url.cc",
     "favicon_url.h",
     "favicon_util.cc",
     "favicon_util.h",
     "large_icon_service.cc",
     "large_icon_service.h",
   ]
 
+  public_deps = [

[sdefresne, 2017/03/23 10:24:41]

I don't think this whole public_deps is correct. It looks like we generally do
not declare public_deps on //base or other targets just because there are
#include of the target in headers.

It looks like they are used either when trying to masquerade a collection of
targets appear like a single target (usually in templates) or when one of the
depended-on target has public_configs.

At least, it appears to be incorrect for //base (it is not part of
//components/favicon/core and does not export public_configs), and probably for
the other targets too for the same reason. I think they should stay in deps and
instead the dependencies fixed in the client code.

[mastiz, 2017/03/23 12:03:23]

Done.

Although not many, there are certainly examples of //base in public_deps,
introduced by senior engineers, e.g.:
https://cs.chromium.org/chromium/src/gin/BUILD.gn?type=cs&q=public_deps&l=98

From the official documentation
(https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...):

"Generally if you are writing a target B and you include C's headers as part of
B's public headers, or targets depending on B should consider B and C to be part
of a unit, you should use public_deps instead of deps."

My interpretation is that the first condition is sufficient (i.e. "you include
C's headers as part of B's public headers"), to use public_deps.

Having said that, while searching for information, I came across one of your
patches:
https://r882-11cdd11fc1da-maruel-dot-chromiumcodereview.appspot.com/2539143002/

Which suggests you've thought this through, so addressed your suggestion.

+    "//base",
+    "//components/favicon_base",
+    "//components/image_fetcher/core",
+    "//components/keyed_service/core",
+  ]
+
   deps = [
-    "//base",
     "//base:i18n",
     "//components/bookmarks/browser",
-    "//components/favicon_base",
+    "//components/data_use_measurement/core",
     "//components/history/core/browser",
-    "//components/keyed_service/core",
     "//net:net",
     "//skia",
     "//ui/base",
     "//ui/gfx",
     "//url",
   ]
   if (!is_ios) {
     deps += [ "//cc/paint" ]
     sources += [
       "fallback_icon_client.h",
@@ skipping 13 matching lines @@
   ]
 
   deps = [
     ":core",
     "//base",
     "//base/test:test_support",
     "//components/favicon/core/test:test_support",
     "//components/favicon_base",
     "//components/history/core/browser:browser",
     "//components/history/core/test:test",
+    "//components/image_fetcher/core",
     "//skia",
     "//testing/gmock",
     "//testing/gtest",
     "//ui/base",
     "//ui/gfx",
     "//ui/gfx:test_support",
     "//url",
   ]
 }