[ReadingList iOS] Do not store HTTP images when distilling HTTPS pages

ios/chrome/browser/reading_list/url_downloader.cc

 // 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/reading_list/url_downloader.h"
 
 #include <string>
 #include <vector>
 
 #include "base/files/file_path.h"
@@ skipping 230 matching lines @@
     const std::string& html,
     const std::vector<dom_distiller::DistillerViewerInterface::ImageInfo>&
         images) {
   std::string mutable_html = html;
   bool local_images_found = false;
   for (size_t i = 0; i < images.size(); i++) {
     if (images[i].url.SchemeIs(url::kDataScheme)) {
       // Data URI, the data part of the image is empty, no need to store it.
       continue;
     }
-    base::FilePath local_image_path;
-    std::string local_image_name;
-    if (!SaveImage(url, images[i].url, images[i].data, &local_image_name)) {
-      return std::string();
+    std::string local_image_name = "";

[jif, 2017/01/20 10:45:26]

afaik you don't need to initialize local_image_name

[Olivier, 2017/01/20 10:54:44]

Done.

+    if (!(distilled_url_.SchemeIsCryptographic() &&

[jif, 2017/01/20 10:45:26]

Because it confused me a bit, can you rewrite it as:

 <new line>
// Only save images if it is not mixed content.
// Mixed content is HTTP images on HTTPS pages.
bool image_is_mixed_content = distilled_url_.SchemeIsCryptographic() &&
!images[i].url.SchemeIsCryptographic();
if (!image_is_mixed_content) {

[Olivier, 2017/01/20 10:54:44]

Done.

+          !images[i].url.SchemeIsCryptographic())) {
+      // Only save images if it is not mixed content.
+      // Mixed content is HTTP imaged on HTTPS pages.

[jif, 2017/01/20 10:45:26]

s/imaged/images/

I appreciate the reminder of what is "mixed content".

[Olivier, 2017/01/20 10:54:44]

Done.

+      if (!SaveImage(url, images[i].url, images[i].data, &local_image_name)) {
+        return std::string();

[jif, 2017/01/20 10:45:26]

Optional nit (because not introduced by your CL, and not related to this CL's
change):

Returning an empty updated html seems wrong; it sounds as though the distilled
page will be blank if it contains mixed content images.
Can you add an explanation, maybe in the method's comment in the header?

[Olivier, 2017/01/20 10:54:44]

Returning the html empty will result in a distillation failure, which is an
expected behavior.
Adding a comment in header.

+      }
     }
     std::string image_url = net::EscapeForHTML(images[i].url.spec());
     size_t image_url_size = image_url.size();
     size_t pos = mutable_html.find(image_url, 0);
     while (pos != std::string::npos) {
       local_images_found = true;
       mutable_html.replace(pos, image_url_size, local_image_name);
       pos = mutable_html.find(image_url, pos + local_image_name.size());
     }
   }
   if (local_images_found) {
     mutable_html += kDisableImageContextMenuScript;
   }
 
   return mutable_html;
 }
 
 bool URLDownloader::SaveHTMLForURL(std::string html, const GURL& url) {
   if (html.empty()) {
     return false;
   }
   base::FilePath path =
       reading_list::OfflinePageAbsolutePath(base_directory_, url);
   return base::WriteFile(path, html.c_str(), html.length()) > 0;
 }