Save favicon during reading list distillation

components/dom_distiller/ios/distiller_page_ios.mm

Index: components/dom_distiller/ios/distiller_page_ios.mm
diff --git a/components/dom_distiller/ios/distiller_page_ios.mm b/components/dom_distiller/ios/distiller_page_ios.mm
index a59548ae016bd82f1a14ebf921b252233d3ddbd1..f7a1c5ed3eaa220e9be26d87b47a0fabc69b15cd 100644
--- a/components/dom_distiller/ios/distiller_page_ios.mm
+++ b/components/dom_distiller/ios/distiller_page_ios.mm
@@ -13,10 +13,14 @@
 #include "base/mac/foundation_util.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/sys_string_conversions.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
-#include "ios/public/provider/web/web_controller_provider.h"
-#include "ios/public/provider/web/web_controller_provider_factory.h"
+#include "components/favicon/ios/web_favicon_driver.h"
 #include "ios/web/public/browser_state.h"
+#import "ios/web/public/navigation_manager.h"
+#include "ios/web/public/web_state/web_state.h"

[Eugene But (OOO till 7-30), 2016/12/08 15:34:53]

s/include/import

[kkhorimoto, 2016/12/08 22:15:24]

More of a comment for Eugene than Gauthier, but it looks like web_state.mm
doesn't actually use any ObjC.  Should we just update it to .cc file and use
#include in the future?

[Eugene But (OOO till 7-30), 2016/12/08 22:45:34]

Header has Objective-C code:
https://cs.chromium.org/chromium/src/ios/web/public/web_state/web_state.h?q=w...

[gambard, 2016/12/12 15:04:26]

Done.

+#import "ios/web/public/web_state/js/crw_js_injection_receiver.h"
+#import "ios/web/public/web_state/js/crw_js_injection_manager.h"
 
 namespace {
 
@@ -125,8 +129,9 @@ void DistillerWebStateObserver::PageLoaded(
 
 #pragma mark -
 
-DistillerPageIOS::DistillerPageIOS(web::BrowserState* browser_state)
-    : browser_state_(browser_state), weak_ptr_factory_(this) {
+DistillerPageIOS::DistillerPageIOS(web::WebState* web_state)
+    : web_state_(web_state), weak_ptr_factory_(this) {
+  web_state_observer_.reset(new DistillerWebStateObserver(web_state_, this));

[kkhorimoto, 2016/12/08 22:15:24]

DCHECK(web_state) before this line?

 }
 
 DistillerPageIOS::~DistillerPageIOS() {
@@ -143,41 +148,41 @@ void DistillerPageIOS::DistillPageImpl(const GURL& url,
   url_ = url;
   script_ = script;
 
-  // Lazily create provider.
-  if (!provider_) {
-    if (ios::GetWebControllerProviderFactory()) {
-      provider_ =
-          ios::GetWebControllerProviderFactory()->CreateWebControllerProvider(
-              browser_state_);
-      web_state_observer_.reset(
-          new DistillerWebStateObserver(provider_->GetWebState(), this));
-    }
-  }
-
-  // Load page using provider.
-  if (provider_)
-    provider_->LoadURL(url_);
-  else
+  // Load page using WebState.
+  if (web_state_) {
+    favicon::WebFaviconDriver* favicon_driver =
+        favicon::WebFaviconDriver::FromWebState(web_state_);
+    favicon_driver->FetchFavicon(url);

[Olivier, 2016/12/08 18:28:02]

favicon_driver->AddObserver(this);

+
+    web::NavigationManager::WebLoadParams params(url);
+    web_state_->SetWebUsageEnabled(true);
+    web_state_->GetNavigationManager()->LoadURLWithParams(params);
+    web_state_->GetView();

[Eugene But (OOO till 7-30), 2016/12/08 15:34:53]

nit: It worth commenting why you have to call this method (this is more like a
workaround).

[gambard, 2016/12/12 15:04:26]

Done.

+  } else

[Eugene But (OOO till 7-30), 2016/12/08 15:34:53]

nit: Needs braces, because previous block has them

[gambard, 2016/12/12 15:04:26]

Done.

     OnLoadURLDone(web::PageLoadCompletionStatus::FAILURE);
 }
 
 void DistillerPageIOS::OnLoadURLDone(
     web::PageLoadCompletionStatus load_completion_status) {
   // Don't attempt to distill if the page load failed or if there is no
-  // provider.
+  // WebState.
   if (load_completion_status == web::PageLoadCompletionStatus::FAILURE ||
-      !provider_) {
+      !web_state_) {
     HandleJavaScriptResult(nil);
     return;
   }
 
   // Inject the script.
   base::WeakPtr<DistillerPageIOS> weak_this = weak_ptr_factory_.GetWeakPtr();
-  provider_->InjectScript(script_, ^(id result, NSError* error) {
-    DistillerPageIOS* distiller_page = weak_this.get();
-    if (distiller_page)
-      distiller_page->HandleJavaScriptResult(result);
-  });
+
+  [[web_state_->GetJSInjectionReceiver()

[Eugene But (OOO till 7-30), 2016/12/08 15:34:53]

Ideally you want to use WebState::ExecuteJavaScript here if possible.
InjectionReceiver exists for historical reasons and we should get rid of it at
some point. 

[gambard, 2016/12/12 15:04:26]

I can use it because WebState::ExecuteJavaScript returns a base::Value result
and I need to extract the value using the script of this page. See
https://codereview.chromium.org/2327783002 for context.

+      instanceOfClass:[CRWJSInjectionManager class]]
+      executeJavaScript:base::SysUTF8ToNSString(script_)
+      completionHandler:^(id result, NSError* error) {
+        DistillerPageIOS* distiller_page = weak_this.get();
+        if (distiller_page)
+          distiller_page->HandleJavaScriptResult(result);
+      }];
 }
 
 void DistillerPageIOS::HandleJavaScriptResult(id result) {