Favicon url is used only if it is valid

ios/web/web_state/ui/crw_web_controller.mm

Index: ios/web/web_state/ui/crw_web_controller.mm
diff --git a/ios/web/web_state/ui/crw_web_controller.mm b/ios/web/web_state/ui/crw_web_controller.mm
index 8b17115ffff239e137ea52f7dc8c6d7228fe2a37..21150c262e09b218e31b8d7db9fa98a370238de7 100644
--- a/ios/web/web_state/ui/crw_web_controller.mm
+++ b/ios/web/web_state/ui/crw_web_controller.mm
@@ -2542,6 +2542,7 @@ registerLoadRequestForURL:(const GURL&)requestURL
     return NO;
   }
   std::vector<web::FaviconURL> urls;
+  BOOL hasFavicon = NO;
   for (size_t fav_idx = 0; fav_idx != favicons->GetSize(); ++fav_idx) {
     base::DictionaryValue* favicon = nullptr;
     if (!favicons->GetDictionary(fav_idx, &favicon))
@@ -2556,13 +2557,36 @@ registerLoadRequestForURL:(const GURL&)requestURL
       DLOG(WARNING) << "JS message parameter not found: rel";
       return NO;
     }
+    BOOL isAppleTouch = YES;
     web::FaviconURL::IconType icon_type = web::FaviconURL::FAVICON;
     if (rel == "apple-touch-icon")
       icon_type = web::FaviconURL::TOUCH_ICON;
     else if (rel == "apple-touch-icon-precomposed")
       icon_type = web::FaviconURL::TOUCH_PRECOMPOSED_ICON;
-    urls.push_back(
-        web::FaviconURL(GURL(href), icon_type, std::vector<gfx::Size>()));
+    else
+      isAppleTouch = NO;
+    GURL url = GURL(href);
+    if (url.is_valid()) {
+      urls.push_back(web::FaviconURL(url, icon_type, std::vector<gfx::Size>()));
+      hasFavicon = hasFavicon || !isAppleTouch;
+    }
+  }
+  if (!hasFavicon) {

[Eugene But (OOO till 7-30), 2017/05/17 14:18:19]

Do you need this variable? Do you want to use |if (urls.empty())| instead and
then remove |if (!urls.empty())| check?

[gambard, 2017/05/17 16:05:37]

The javascript code added the default favicon only if it was non apple touch. I
did the same behavior here (i.e. the urls might not be empty).

[Eugene But (OOO till 7-30), 2017/05/17 18:30:18]

Thanks for explanation. Could you please specify that in the comments.

[gambard, 2017/05/18 07:20:56]

Done.

+    id origin = context[kOriginURLKey];
+    if (origin) {
+      NSURL* originURL = base::mac::ObjCCastStrict<NSURL>(origin);

[Eugene But (OOO till 7-30), 2017/05/17 14:18:19]

Could you please convert this to GURL and use GURL API instead. In Chromium we
avoid using NSURL unless it should be passed to SDK methods.

[Eugene But (OOO till 7-30), 2017/05/17 14:18:19]

Do you want to move comments from JS which explained this code?

[gambard, 2017/05/17 16:05:37]

Done.

[gambard, 2017/05/17 16:05:37]

Done.

+      if ([originURL.scheme isEqualToString:@"http"] ||
+          [originURL.scheme isEqualToString:@"https"]) {
+        NSURLComponents* URLComponents = [[NSURLComponents alloc] init];
+        URLComponents.scheme = originURL.scheme;
+        URLComponents.host = originURL.host;
+        URLComponents.path = @"/favicon.ico";
+        NSURL* defaultFaviconURL = URLComponents.URL;
+        urls.push_back(web::FaviconURL(net::GURLWithNSURL(defaultFaviconURL),
+                                       web::FaviconURL::FAVICON,
+                                       std::vector<gfx::Size>()));
+      }
+    }
   }
   if (!urls.empty())
     _webStateImpl->OnFaviconUrlUpdated(urls);