Save favicon during reading list distillation

ios/chrome/browser/dom_distiller/distiller_favicon.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/distiller_favicon.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"
+#include "ios/public/provider/web/web_controller_provider.h"
+#include "ios/web/public/favicon_url.h"
+#include "url/gurl.h"
+
+namespace dom_distiller {
+
+DistillerFavicon::DistillerFavicon(web::BrowserState* browser_state)
+    : browser_state_(browser_state) {}
+
+DistillerFavicon::~DistillerFavicon() {}
+
+void DistillerFavicon::HandleFaviconURL(std::vector<web::FaviconURL> urls,
+                                        const GURL& page_url) {
+  if (urls.empty())
+    return;
+
+  ios::ChromeBrowserState* original_browser_state =
+      ios::ChromeBrowserState::FromBrowserState(browser_state_);
+
+  std::unique_ptr<ios::WebControllerProvider> provider =
+      ios::GetWebControllerProviderFactory()->CreateWebControllerProvider(

[kkhorimoto, 2016/11/28 23:48:25]

This is creating a new WebState, so this will be different than the WebState
being used to load the page/run the distillation scripts.

+          browser_state_);
+
+  web::WebState* web_state = provider->GetWebState();

[gambard, 2016/11/28 10:20:04]

This might not be the good way to get the web_state, it is the same as in the
distiller_page_ios.

[kkhorimoto, 2016/11/28 23:48:25]

Ideally, we should be removing the web controller provider altogether; I'm
pretty sure that all the necessary functionality is already present in
WebState's public interface at this point.  For now, accessing like this is
fine.

[Eugene But (OOO till 7-30), 2016/11/30 18:51:31]

There should be no need to create new WebControllerProvider. You can just create
a new WebState instead.

+
+  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));
+
+  favicon::WebFaviconDriver* favicon_driver =
+      favicon::WebFaviconDriver::FromWebState(web_state);
+
+  favicon_driver->FetchFavicon(page_url);

[kkhorimoto, 2016/11/28 23:48:26]

Is this doing the same as executing the fetch favicon script from common.js? 
Why are we fetching here? Can you add comments?

+
+  ((web::WebStateObserver*)favicon_driver)->FaviconUrlUpdated(urls);

[kkhorimoto, 2016/11/28 23:48:25]

I'm unclear on what exactly is happening here.  Couldn't we just create the
WebFaviconDriver before loading the page in DistillerPageIOS?  That way, it will
automatically receive this WebStateObserver callback when the page loads
initially.

[kkhorimoto, 2016/11/28 23:48:26]

Also, we need to be using static_cast<> for these types of casting operations.

[gambard, 2016/11/30 10:59:43]

You are right, but it is because this code isn't working (I have been unlucky
when testing it).
The implementation of FaviconUrlUpdated in web_favicon_driver ask for
GetActiveURL() which uses web_state()->GetNavigationManager()->GetVisibleItem()
which is not set in my case.

The method I really want to call (and fixing the code) is
FaviconDriverImpl::OnUpdateFaviconURL() in which I can set the url.

+}
+}