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 kDefaultDelayFaviconSecond = 10;
+}
+
+namespace dom_distiller {
+
+FaviconWebStateDispatcherImpl::FaviconWebStateDispatcherImpl(
+    web::BrowserState* browser_state)
+    : FaviconWebStateDispatcherImpl(browser_state, -1) {}
+
+FaviconWebStateDispatcherImpl::FaviconWebStateDispatcherImpl(
+    web::BrowserState* browser_state,
+    int keep_alive_time_second)
+    : FaviconWebStateDispatcher(),
+      browser_state_(browser_state),
+      keep_alive_time_second_(keep_alive_time_second),

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

How about this?:
keep_alive_time_second_(kDefaultDelayFaviconSecond)

This way you can drop |keep_alive_time_second_ < 0| ternary. Otherwise unittests
tests code path that is not running in production.

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

I think you wanted to write:

  keep_alive_time_second_(
      keep_alive_time_second < 0
          ? kDefaultDelayFaviconSecond
          : keep_alive_time_second),

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

Sorry. I wanted to suggest changing the first constructor:

FaviconWebStateDispatcherImpl::FaviconWebStateDispatcherImpl(
  web::BrowserState* browser_state)
  : FaviconWebStateDispatcherImpl(browser_state, kDefaultDelayFaviconSecond) {}

and drop the usage or ternary operator. Or just fully drop the first constructor
and make all clients pass kDefaultDelayFaviconSecond as an argument.

+      weak_ptr_factory_(this) {}
+
+FaviconWebStateDispatcherImpl::~FaviconWebStateDispatcherImpl() {}
+
+web::WebState* FaviconWebStateDispatcherImpl::RequestWebState() {
+  const web::WebState::CreateParams web_state_create_params(browser_state_);
+  std::unique_ptr<web::WebState> web_state_unique =

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

To avoid creating two local variables, I would do (simpler, no move, no
web_state_uniwe, ...):

  web_states_.push_back(web::WebState::Create(web_state_create_params));
  web::WebState* web_state = web_state_.back();

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

Acknowledged.

+      web::WebState::Create(web_state_create_params);
+  web::WebState* web_state = web_state_unique.get();
+
+  web_states_.push_back(std::move(web_state_unique));
+
+  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_second_ < 0 ? kDefaultDelayFaviconSecond

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

I like eugenebut@ suggestion of doing the check in the constructor since this
value cannot be changed after that point. To make the dispatch_after cleaner, I
would still use a local variable here for the cast:

  int64_t keep_alive_nsec = static_cast<int64_t>(keep_alive_time_second_) *
NSEC_PER_SEC;
  dispatch_after(dispatch_time(DISPATCH_TIME_NOW, keep_alive_nsec), ...

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

Done.
Making the constructor take a int64_t as argument.

+                                                  : keep_alive_time_second_;
+  dispatch_after(
+      dispatch_time(DISPATCH_TIME_NOW,
+                    static_cast<int64_t>(callback_time * NSEC_PER_SEC)),
+      dispatch_get_main_queue(), ^{
+        FaviconWebStateDispatcherImpl* web_state_dispatcher = weak_this.get();
+        if (web_state_dispatcher) {
+          auto it = find_if(
+              web_state_dispatcher->web_states_.begin(),
+              web_state_dispatcher->web_states_.end(),
+              [web_state](std::unique_ptr<web::WebState>& unique_web_state) {
+                return unique_web_state.get() == web_state;
+              });

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

I think you can DCHECK here:

  DCHECK(it != web_state_dispatcher->web_states_.end());
  web_state_dispatcher->web_states_.erase(it);

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

Acknowledged.

+          if (it != web_state_dispatcher->web_states_.end())
+            web_state_dispatcher->web_states_.erase(it);
+        }
+      });
+}
+
+}  // namespace dom_distiller