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

chrome/browser/resources/history/history.js

 // Copyright (c) 2012 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 src="../uber/uber_utils.js">
 <include src="history_focus_manager.js">
 
 ///////////////////////////////////////////////////////////////////////////////
 // Globals:
 /** @const */ var RESULTS_PER_PAGE = 150;
@@ skipping 39 matching lines @@
  * chrome/browser/ui/webui/history_ui.cc:
  *     BrowsingHistoryHandler::HistoryEntry::ToValue()
  * @typedef {{allTimestamps: Array<number>,
  *            blockedVisit: (boolean|undefined),
  *            dateRelativeDay: (string|undefined),
  *            dateShort: string,
  *            dateTimeOfDay: (string|undefined),
  *            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,
  *            time: number,
  *            title: string,
  *            url: string}}
  */
 var HistoryEntry;
 
 /**
@@ skipping 48 matching lines @@
  *     visit before it.
  * @param {HistoryModel} model The model object this entry belongs to.
  * @constructor
  */
 function Visit(result, continued, model) {
   this.model_ = model;
   this.title_ = result.title;
   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.
   this.deviceName = result.deviceName;
   this.deviceType = result.deviceType;
 
   // The ID will be set according to when the visit was displayed, not
   // received. Set to -1 to show that it has not been set yet.
   this.id_ = -1;
 
@@ skipping 107 matching lines @@
       bookmarkSection.removeEventListener('click', f);
       e.preventDefault();
     }.bind(this));
   }
 
   if (focusless)
     bookmarkSection.tabIndex = -1;
 
   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)
     visitEntryWrapper.classList.add('visit-entry');
   if (this.blockedVisit) {
     visitEntryWrapper.classList.add('blocked-indicator');
     visitEntryWrapper.appendChild(this.getVisitAttemptDOM_());
   } else {
     var title = visitEntryWrapper.appendChild(
         this.getTitleDOM_(isSearchResult));
 
-    if (addTitleFavicon)
-      this.addFaviconToElement_(visitEntryWrapper);
-
     if (focusless)
       title.querySelector('a').tabIndex = -1;
 
     visitEntryWrapper.appendChild(domain);
   }
 
   if (isMobileVersion()) {
     if (this.model_.editingEntriesAllowed) {
       var removeButton = createElementWithClassName('button', 'remove-entry');
       removeButton.setAttribute('aria-label',
@@ skipping 187 matching lines @@
 Visit.prototype.getVisitAttemptDOM_ = function() {
   var node = createElementWithClassName('div', 'title');
   node.innerHTML = loadTimeData.getStringF('blockedVisitText',
                                            this.url_,
                                            this.id_,
                                            this.domain_);
   return node;
 };
 
 /**
- * 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.addFaviconToElement_ = function(el) {
-  var url = isMobileVersion() ?
-      getFaviconImageSet(this.url_, 32, 'touch-icon') :
-      getFaviconImageSet(this.url_);
-  el.style.backgroundImage = url;
+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.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);
+};
+
+/**
  * Launch a search for more history entries from the same domain.
  * @private
  */
 Visit.prototype.showMoreFromSite_ = function() {
   recordUmaAction('HistoryPage_EntryMenuShowMoreFromSite');
   historyView.setSearch(this.domain_);
   $('search-field').focus();
 };
 
 /**
@@ skipping 844 matching lines @@
 
 /**
  * Generates and adds the grouped visits DOM for a certain domain. This
  * includes the clickable arrow and domain name and the visit entries for
  * that domain.
  * @param {Element} results DOM object to which to add the elements.
  * @param {string} domain Current domain name.
  * @param {Array} domainVisits Array of visits for this domain.
  * @private
  */
 HistoryView.prototype.getGroupedVisitsDOM_ = function(

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

ah, you did check the grouped view :) sorry for not noticing you updated this
code already

     results, domain, domainVisits) {
   // Add a new domain entry.
   var siteResults = results.appendChild(
       createElementWithClassName('li', 'site-entry'));
 
   var siteDomainWrapper = siteResults.appendChild(
       createElementWithClassName('div', 'site-domain-wrapper'));
   // Make a row that will contain the arrow, the favicon and the domain.
   var siteDomainRow = siteDomainWrapper.appendChild(
       createElementWithClassName('div', 'site-domain-row'));
 
   if (this.model_.editingEntriesAllowed) {
     var siteDomainCheckbox =
         createElementWithClassName('input', 'domain-checkbox');
 
     siteDomainCheckbox.type = 'checkbox';
     siteDomainCheckbox.addEventListener('click', domainCheckboxClicked);
     siteDomainCheckbox.domain_ = domain;
     siteDomainCheckbox.setAttribute('aria-label', domain);
     siteDomainRow.appendChild(siteDomainCheckbox);
   }
 
   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);
   siteDomainLink.textContent = domain;
   var numberOfVisits = createElementWithClassName('span', 'number-visits');
   var domainElement = document.createElement('span');
 
   numberOfVisits.textContent = loadTimeData.getStringF('numberVisits',
                                                        domainVisits.length);
   siteDomain.appendChild(numberOfVisits);
 
-  domainVisits[0].addFaviconToElement_(siteDomain);
+  domainVisits[0].loadFavicon_(siteFavicon);
 
   siteDomainWrapper.addEventListener(
       'click', this.toggleGroupedVisits_.bind(this));
 
   if (this.model_.isSupervisedProfile) {
     siteDomainRow.appendChild(
         getFilteringStatusDOM(domainVisits[0].hostFilteringBehavior));
   }
 
   siteResults.appendChild(siteDomainWrapper);
@@ skipping 980 matching lines @@
     historyView.reload();
 }
 
 // Add handlers to HTML elements.
 document.addEventListener('DOMContentLoaded', load);
 
 // This event lets us enable and disable menu items before the menu is shown.
 document.addEventListener('canExecute', function(e) {
   e.canExecute = true;
 });