[Android] Use fallback icon if there is no large favicon on the history page

chrome/browser/resources/history/history.js

Index: chrome/browser/resources/history/history.js
diff --git a/chrome/browser/resources/history/history.js b/chrome/browser/resources/history/history.js
index 3ea486d1f9acf527550d3a3c5a61a9d224952454..7f646f78cdf864817457fc793baa524ab802493b 100644
--- a/chrome/browser/resources/history/history.js
+++ b/chrome/browser/resources/history/history.js
@@ -57,6 +57,7 @@ var SupervisedUserFilteringBehavior = {
  *            deviceName: string,
  *            deviceType: string,
  *            domain: string,
+ *            fallbackFaviconText: string,

[Dan Beam, 2016/02/05 20:42:11]

is this ever more than a character?

[pkotwicz, 2016/02/06 02:59:55]

Yes, fallbackFaviconText is "IP" for IP urls. Otherwise, this always a single
character

  *            hostFilteringBehavior: (number|undefined),
  *            snippet: (string|undefined),
  *            starred: boolean,
@@ -125,6 +126,7 @@ function Visit(result, continued, model) {
   this.url_ = result.url;
   this.domain_ = result.domain;
   this.starred_ = result.starred;
+  this.fallbackFaviconText_ = result.fallbackFaviconText;
 
   // These identify the name and type of the device on which this visit
   // occurred. They will be empty if the visit occurred on the current device.
@@ -252,6 +254,15 @@ Visit.prototype.getResultDOM = function(propertyBag) {
 
   entryBox.appendChild(bookmarkSection);
 
+  if (addTitleFavicon || this.blockedVisit) {
+    var faviconSection = createElementWithClassName('div', 'favicon');
+    if (this.blockedVisit)
+      faviconSection.classList.add('blocked-icon');
+    else
+      this.loadFavicon_(faviconSection);
+    entryBox.appendChild(faviconSection);
+  }
+
   var visitEntryWrapper = /** @type {HTMLElement} */(
       entryBox.appendChild(document.createElement('div')));
   if (addTitleFavicon || this.blockedVisit)
@@ -263,9 +274,6 @@ Visit.prototype.getResultDOM = function(propertyBag) {
     var title = visitEntryWrapper.appendChild(
         this.getTitleDOM_(isSearchResult));
 
-    if (addTitleFavicon)
-      this.addFaviconToElement_(visitEntryWrapper);
-
     if (focusless)
       title.querySelector('a').tabIndex = -1;
 
@@ -473,15 +481,42 @@ Visit.prototype.getVisitAttemptDOM_ = function() {
 };
 
 /**
- * Set the favicon for an element.
- * @param {Element} el The DOM element to which to add the icon.
+ * Load the favicon for an element.
+ * @param {Element} faviconDiv The DOM element for which to load the icon.
+ * @private
+ */
+Visit.prototype.loadFavicon_ = function(faviconDiv) {
+  if (cr.isAndroid) {
+    // On Android, if a large icon is unavailable, a fallback favicon is
+    // generated in Javascript because Android does not yet support text

[newt (away), 2016/01/25 19:43:35]

Maybe: "an HTML/CSS fallback favicon is generated"? "generated in JavaScript"
makes me think that someone has written a rendering engine in JavaScript.

[pkotwicz, 2016/02/06 02:59:55]

Done.

+    // drawing in native.

[Dan Beam, 2016/02/05 20:42:12]

well, that's not really true... 
https://developer.mozilla.org/en-US/docs/Web/API/Canvas_API/Tutorial/Drawing_...

[pkotwicz, 2016/02/06 02:59:55]

I was referring to Android using canvas_notimplemented.cc Notice that all of the
text drawing methods are stubbed out. Is there a clearer way of wording this
comment?

I know that it is possible to do text drawing via JavaScript canvas and via Java
(and whatever way that blink does it via C++)

+
+    // Check whether a fallback favicon needs to be generated.
+    var desiredPixelSize = 32 * window.devicePixelRatio;
+    var img = new Image();
+    img.onload = this.onLargeFaviconLoadedAndroid_.bind(this, faviconDiv, img);

[Dan Beam, 2016/02/05 20:42:11]

you probably don't need to bind img because you can use e.target or
e.currentTarget from the handler

[Dan Beam, 2016/02/05 20:42:12]

can we just trigger onerror in the case we fail to load a favicon?

[pkotwicz, 2016/02/06 02:59:55]

Currently, when there is no large enough favicon, the load succeeds. A bitmap
with the dominant color of the small icon is returned instead of the large icon.

How do you suggest that I send the dominant color of the small favicon to
JavaScript if I change the logic to "return null if there is no large enough
favicon"? I do not want to send the dominant color of the small icon in response
to BrowsingHistoryHandler::QueryHistory() because getting the small favicon
requires multiple database requests.

+    img.src = 'chrome://large-icon/' + desiredPixelSize + '/' + this.url_;
+  } else {
+    faviconDiv.style.backgroundImage = getFaviconImageSet(this.url_);
+  }
+};
+
+/**
+ * Called when the chrome://large-icon image has finished loading.
+ * @param {Element} faviconDiv The DOM element to add the favicon to.
+ * @param {HTMLImageElement} loadedImg The image which finished loading.
  * @private
  */
-Visit.prototype.addFaviconToElement_ = function(el) {
-  var url = isMobileVersion() ?
-      getFaviconImageSet(this.url_, 32, 'touch-icon') :
-      getFaviconImageSet(this.url_);
-  el.style.backgroundImage = url;
+Visit.prototype.onLargeFaviconLoadedAndroid_ = function(faviconDiv, loadedImg) {
+  // The loaded image should either:
+  // - Have the desired size.
+  // OR
+  // - Be 1x1 px with the background color for the fallback icon.
+  if (loadedImg.width == 1 && loadedImg.height == 1) {

[Dan Beam, 2016/02/05 20:42:12]

is it likely that width == 1 and height != 1?

[pkotwicz, 2016/02/06 02:59:55]

It is not likely. I changed the if() to check just the width.

+    faviconDiv.classList.add('fallback-favicon');

[Dan Beam, 2016/02/05 20:42:11]

can we just create a wrapper div *if* the icon fails to load?

[pkotwicz, 2016/02/06 02:59:55]

Creating the wrapper div only if the icon fails to load sounds pretty
complicated.

If the icon fails to load:
- I would need to have a reference to the visit-entry div.
- I would need to change the visit-entry div's classList so that the visit-entry
div has 0px left padding.
- I would need to insert the favicon wrapper div before the visit-entry div
using HTMLDOMElement.insertBefore().

Is there an easier way?

+    faviconDiv.innerHTML = this.fallbackFaviconText_;

[Dan Beam, 2016/02/05 20:42:11]

can you use textContent instead?  (innerHTML is bad for security in general)

[pkotwicz, 2016/02/06 02:59:55]

Done.

+  }
+  faviconDiv.style.backgroundImage = url(loadedImg.src);
 };
 
 /**
@@ -1374,6 +1409,8 @@ HistoryView.prototype.getGroupedVisitsDOM_ = function(
 
   var siteArrow = siteDomainRow.appendChild(
       createElementWithClassName('div', 'site-domain-arrow'));
+  var siteFavicon = siteDomainRow.appendChild(
+      createElementWithClassName('div', 'favicon'));

[Dan Beam, 2016/02/05 20:42:12]

is it necessary to add this wrapper on desktop?

   var siteDomain = siteDomainRow.appendChild(
       createElementWithClassName('div', 'site-domain'));
   var siteDomainLink = siteDomain.appendChild(new ActionLink);
@@ -1385,7 +1422,7 @@ HistoryView.prototype.getGroupedVisitsDOM_ = function(
                                                        domainVisits.length);
   siteDomain.appendChild(numberOfVisits);
 
-  domainVisits[0].addFaviconToElement_(siteDomain);
+  domainVisits[0].loadFavicon_(siteFavicon);
 
   siteDomainWrapper.addEventListener(
       'click', this.toggleGroupedVisits_.bind(this));