Fix domdistiller for new JS execution

components/dom_distiller/ios/distiller_page_ios.mm

 // Copyright 2014 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 "components/dom_distiller/ios/distiller_page_ios.h"
 
 #import <UIKit/UIKit.h>
 
 #include <utility>
 
 #include "base/json/json_reader.h"
 #include "base/logging.h"
+#include "base/memory/ptr_util.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/values.h"
 #include "ios/public/provider/web/web_controller_provider.h"
 #include "ios/web/public/browser_state.h"
 
 namespace dom_distiller {
 
 // Helper class for observing the loading of URLs to distill.
 class DistillerWebStateObserver : public web::WebStateObserver {
  public:
@@ skipping 24 matching lines @@
 #pragma mark -
 
 DistillerPageIOS::DistillerPageIOS(web::BrowserState* browser_state)
     : browser_state_(browser_state), weak_ptr_factory_(this) {
 }
 
 DistillerPageIOS::~DistillerPageIOS() {
 }
 
 bool DistillerPageIOS::StringifyOutput() {
- // UIWebView requires JavaScript to return a single string value.
- return true;
+  return false;
 }
 
 void DistillerPageIOS::DistillPageImpl(const GURL& url,
                                        const std::string& script) {
   if (!url.is_valid() || !script.length())
     return;
   url_ = url;
   script_ = script;
 
   // Lazily create provider.
@@ skipping 13 matching lines @@
   else
     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.
   if (load_completion_status == web::PageLoadCompletionStatus::FAILURE ||
       !provider_) {
-    HandleJavaScriptResultString(@"");
+    HandleJavaScriptResult(nil);
     return;
   }
 
   // Inject the script.
   base::WeakPtr<DistillerPageIOS> weak_this = weak_ptr_factory_.GetWeakPtr();
-  provider_->InjectScript(script_, ^(NSString* string, NSError* error) {
+  provider_->InjectScript(script_, ^(id result, NSError* error) {
     DistillerPageIOS* distiller_page = weak_this.get();
     if (distiller_page)
-      distiller_page->HandleJavaScriptResultString(string);
+      distiller_page->HandleJavaScriptResult(result);
   });
 }
 
-void DistillerPageIOS::HandleJavaScriptResultString(NSString* result) {
+void DistillerPageIOS::HandleJavaScriptResult(id result) {
   std::unique_ptr<base::Value> resultValue = base::Value::CreateNullValue();
-  if (result.length) {
-    std::unique_ptr<base::Value> dictionaryValue =
-        base::JSONReader::Read(base::SysNSStringToUTF8(result));
-    if (dictionaryValue &&
-        dictionaryValue->IsType(base::Value::TYPE_DICTIONARY)) {
-      resultValue = std::move(dictionaryValue);
-    }
+  if (result) {
+    resultValue = ValueResultFromWKResult(result);
   }
   OnDistillationDone(url_, resultValue.get());
 }
 
+// This is duplicated here from ios/web/web_state/ui/web_view_js_utils.mm in
+// order to handle numbers. The dom distiller proto expects integers and the
+// generated JSON deserializer does not accept doubles in the place of ints.
+// However WKWebView only allows us to return "numbers." We need to create
+// integers and doubles as expected by the proto, which is done by checking if
+// the number has a fraction or not; since this is a hacky method it's isolated
+// to this file so as to limit the risk of broken JS calls.
+std::unique_ptr<base::Value> DistillerPageIOS::ValueResultFromWKResult(
+    id wk_result) {
+  if (!wk_result)
+    return nullptr;

[Eugene But (OOO till 7-30), 2016/09/09 15:28:28]

Optional NIT: You can return |base::Value::CreateNullValue()| here and drop
check on line 107

[lody, 2016/09/19 15:34:36]

I'd rather not have any part of this be different from the original code except
what's necessary; it'll make it easier to read and update.

+
+  std::unique_ptr<base::Value> result;
+  CFTypeID result_type = CFGetTypeID(wk_result);
+  if (result_type == CFStringGetTypeID()) {
+    result.reset(new base::StringValue(base::SysNSStringToUTF16(wk_result)));
+    DCHECK(result->IsType(base::Value::TYPE_STRING));
+  } else if (result_type == CFNumberGetTypeID()) {
+    if ([wk_result intValue] != [wk_result doubleValue]) {
+      result.reset(new base::FundamentalValue([wk_result doubleValue]));
+      DCHECK(result->IsType(base::Value::TYPE_DOUBLE));
+    } else {
+      result.reset(new base::FundamentalValue([wk_result intValue]));
+      DCHECK(result->IsType(base::Value::TYPE_INTEGER));
+    }
+  } else if (result_type == CFBooleanGetTypeID()) {
+    result.reset(
+        new base::FundamentalValue(static_cast<bool>([wk_result boolValue])));
+    DCHECK(result->IsType(base::Value::TYPE_BOOLEAN));
+  } else if (result_type == CFNullGetTypeID()) {
+    result = base::Value::CreateNullValue();
+    DCHECK(result->IsType(base::Value::TYPE_NULL));
+  } else if (result_type == CFDictionaryGetTypeID()) {
+    std::unique_ptr<base::DictionaryValue> dictionary =
+        base::MakeUnique<base::DictionaryValue>();
+    for (id key in wk_result) {
+      DCHECK([key respondsToSelector:@selector(UTF8String)]);
+      const std::string& path([key UTF8String]);

[Eugene But (OOO till 7-30), 2016/09/09 15:28:28]

Please use sys_string_conversions instead of |UTF8String|. I asked author of
original code to fix this as well.

[lody, 2016/09/19 15:34:36]

Done.

+      dictionary->Set(path,
+                      ValueResultFromWKResult([wk_result objectForKey:key]));

[Eugene But (OOO till 7-30), 2016/09/09 15:28:28]

Can this recursion cause a stack overflow?

[Eugene But (OOO till 7-30), 2016/09/09 15:28:28]

NIT: s/[wk_result objectForKey:key]/wk_result[key]

[Eugene But (OOO till 7-30), 2016/09/10 00:23:40]

JsonParser measures stack depth:
https://cs.chromium.org/chromium/src/base/json/json_parser.cc?q=StackMarker&s...

[lody, 2016/09/19 15:34:36]

Duplicated code fixed by jif.

+    }
+    result = std::move(dictionary);
+  } else if (result_type == CFArrayGetTypeID()) {
+    std::unique_ptr<base::ListValue> list = base::MakeUnique<base::ListValue>();
+    for (id value in wk_result) {
+      list->Append(ValueResultFromWKResult(value));

[Eugene But (OOO till 7-30), 2016/09/09 15:28:28]

Same question here

[lody, 2016/09/19 15:34:35]

Acknowledged.

+    }
+    result = std::move(list);
+  } else {
+    NOTREACHED();  // Convert other types as needed.
+  }
+  return result;
+}
+
 }  // namespace dom_distiller