Save favicon during reading list distillation

ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm

+// 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.
+
+#include "ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h"
+
+#include "components/favicon/ios/web_favicon_driver.h"
+#include "components/keyed_service/core/service_access_type.h"
+#include "ios/chrome/browser/bookmarks/bookmark_model_factory.h"
+#include "ios/chrome/browser/browser_state/chrome_browser_state.h"
+#include "ios/chrome/browser/favicon/favicon_service_factory.h"
+#include "ios/chrome/browser/history/history_service_factory.h"
+#import "ios/web/public/web_state/web_state.h"
+
+namespace {
+// Default delay to download the favicon when the WebState is handed back.
+const int kDefaultDelayFavicon = 10;

[sdefresne, 2016/12/20 15:41:00]

Please add unit in the variable name. Like kDefaultDelayFaviconInSeconds.

[gambard, 2016/12/20 17:11:43]

Done.

+}
+
+namespace dom_distiller {
+
+FaviconWebStateDispatcherImpl::FaviconWebStateDispatcherImpl(
+    web::BrowserState* browser_state)
+    : FaviconWebStateDispatcherImpl(browser_state, -1) {}
+FaviconWebStateDispatcherImpl::FaviconWebStateDispatcherImpl(

[sdefresne, 2016/12/20 15:41:00]

Please leave an empty line between methods

[gambard, 2016/12/20 17:11:42]

Done.

+    web::BrowserState* browser_state,
+    int keep_alive_time)
+    : FaviconWebStateDispatcher(),
+      browser_state_(browser_state),
+      keep_alive_time_(keep_alive_time),
+      weak_ptr_factory_(this) {}
+FaviconWebStateDispatcherImpl::~FaviconWebStateDispatcherImpl() {}

[sdefresne, 2016/12/20 15:41:00]

ditto

[gambard, 2016/12/20 17:11:43]

Done.

+
+web::WebState* FaviconWebStateDispatcherImpl::RequestWebState() {
+  const web::WebState::CreateParams web_state_create_params(browser_state_);
+  std::unique_ptr<web::WebState> web_state_unique =
+      web::WebState::Create(web_state_create_params);
+  web::WebState* web_state = web_state_unique.get();
+
+  web_states_[web_state] = std::move(web_state_unique);

[sdefresne, 2016/12/20 15:41:00]

This is really strange. Do you really need a std::map<>? I think a std::vector<>
should be enough (deletion requires a linear search, but I don't think we have
enough WebState for a std::map to outperform linear search in a vector).

[gambard, 2016/12/20 17:11:43]

Done.

+
+  ios::ChromeBrowserState* original_browser_state =
+      ios::ChromeBrowserState::FromBrowserState(browser_state_);
+
+  favicon::WebFaviconDriver::CreateForWebState(
+      web_state,
+      ios::FaviconServiceFactory::GetForBrowserState(
+          original_browser_state, ServiceAccessType::EXPLICIT_ACCESS),
+      ios::HistoryServiceFactory::GetForBrowserState(
+          original_browser_state, ServiceAccessType::EXPLICIT_ACCESS),
+      ios::BookmarkModelFactory::GetForBrowserState(original_browser_state));
+
+  return web_state;
+}
+
+void FaviconWebStateDispatcherImpl::ReturnWebState(web::WebState* web_state) {
+  base::WeakPtr<FaviconWebStateDispatcherImpl> weak_this =
+      weak_ptr_factory_.GetWeakPtr();
+  int callback_time =
+      keep_alive_time_ == -1 ? kDefaultDelayFavicon : keep_alive_time_;
+  dispatch_after(
+      dispatch_time(DISPATCH_TIME_NOW, (int64_t)(callback_time * NSEC_PER_SEC)),

[sdefresne, 2016/12/20 15:41:00]

C style cast are forbidden by the style guide, use static_cast<int64_t>() here.

[gambard, 2016/12/20 17:11:42]

The C style cast is the default snippet for dispatch_after, but Done.

+      dispatch_get_main_queue(), ^{
+        FaviconWebStateDispatcherImpl* web_state_dispatcher = weak_this.get();
+        if (web_state_dispatcher)
+          web_state_dispatcher->web_states_.erase(web_state);
+      });
+}
+
+}  // namespace dom_distiller