Save favicon during reading list distillation

ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h

Index: ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h
diff --git a/ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h b/ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h
new file mode 100644
index 0000000000000000000000000000000000000000..0bd69b88be57c91c40d18104ce7f5f902ac4a882
--- /dev/null
+++ b/ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h
@@ -0,0 +1,46 @@
+// Copyright 2016 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 IOS_CHROME_BROWSER_DOM_DISTILLER_FAVICON_WEB_STATE_DISPATCHER_IMPL_H_
+#define IOS_CHROME_BROWSER_DOM_DISTILLER_FAVICON_WEB_STATE_DISPATCHER_IMPL_H_
+
+#include <memory>
+#include <vector>
+
+#include "base/memory/weak_ptr.h"
+#include "components/dom_distiller/ios/favicon_web_state_dispatcher.h"
+
+namespace web {
+class BrowserState;
+}
+
+namespace dom_distiller {
+
+// Implementation of the FaviconWebStateDispatcher.
+class FaviconWebStateDispatcherImpl : public FaviconWebStateDispatcher {
+ public:
+  // Constructor with default delay for keeping the WebStates alive.
+  explicit FaviconWebStateDispatcherImpl(web::BrowserState* browser_state);
+  // Constructor for keeping the WebStates alive for |keep_alive_time_second|
+  // seconds. If |keep_alive_time_second| < 0 then the default value is used.

[sdefresne, 2016/12/20 18:05:31]

nit: I think |keep_alive_seconds| is better (using unit implies it is a time)

[gambard, 2016/12/21 08:42:52]

Done.

+  FaviconWebStateDispatcherImpl(web::BrowserState* browser_state,
+                                int keep_alive_time_second);

[Eugene But (OOO till 7-30), 2016/12/20 17:36:18]

Do you want to drop |FaviconWebStateDispatcherImpl(web::BrowserState*)| and make
clients always pass timeout? This way tests and production code would use the
same code (which is what we should do whenever possible).

[sdefresne, 2016/12/20 18:05:31]

Another option would be to have a default value for keep_alive_time_second.

  // Constructor for keeping the WebStates alive for |keep_alive_time_second|
  // seconds. If |keep_alive_time_second| < 0 then the default value is used.
  explicit FaviconWebStateDispatcherImpl(web::BrowserState* browser_state,
                                         int keep_alive_time_second = -1);

Then production and test will use the same code path, while production does not
have to specify a timeout.

[Eugene But (OOO till 7-30), 2016/12/20 18:59:02]

Default values are somewhat discouraged by C++ Style Guide and rarely used in
Chromium:
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

[gambard, 2016/12/21 08:42:52]

Done.

+  ~FaviconWebStateDispatcherImpl() override;
+
+  // FaviconWebStateDispatcher implementation.
+  web::WebState* RequestWebState() override;
+  void ReturnWebState(web::WebState* web_state) override;
+
+ private:
+  web::BrowserState* browser_state_;
+  // Map of the WebStates currently alive.
+  std::vector<std::unique_ptr<web::WebState>> web_states_;
+  // Time during which the WebState will be kept alive after being returned.
+  int keep_alive_time_second_;
+  base::WeakPtrFactory<FaviconWebStateDispatcherImpl> weak_ptr_factory_;
+};
+
+}  // namespace dom_distiller
+
+#endif  // IOS_CHROME_BROWSER_DOM_DISTILLER_FAVICON_WEB_STATE_DISPATCHER_IMPL_H_