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

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

This CL makes the history page use a rounded rectangle with the first letter of
the URL for history entries for which there is no large enough favicon.
Previously, the history page did a really bad resize for entries for which there
is no large enough favicon.

This CL:
- Introduces an Android version of FallbackIconService:
FallbackIconServiceImplAndroid which uses RoundedIconGenerator
- Encodes the SkBitmap produced by FallbackIconService to PNG on a worker
  thread. Encoding to PNG takes on average 5ms on a Nexus 5.

BUG=566012
Committed: https://crrev.com/52ce2121706ad270ecf5ee01b400e3f0aabd941b
Cr-Commit-Position: refs/heads/master@{#374247}

[pkotwicz, 2016-01-18 16:57:35]

pkotwicz@chromium.org changed reviewers:
+ newt@chromium.org

[pkotwicz, 2016-01-18 16:57:35]

Newton, can you please take a look?

We will probably want to change the font size and the corner roundness of the
generated history icons in a followup CL.
I used the defaults from FallbackIconStyle which are different from the values
used elsewhere on Android.

[newt (away), 2016-01-19 22:11:05]

Why not just use HTML to render a rounded rectangle with a letter in it? Seems
much simple and way faster than using RoundedIconGenerator to generate Java
Bitmaps and converting those to SkBitmaps, then passing those to the renderer.

I also think we should be careful to find a balance between pretty icons and
useful icons. Using RoundedIconGenerator in too many cases will lead to pretty
but largely useless favicons on the history page; whereas scaling up 16x16
favicons will be more useful but less pretty.

[pkotwicz, 2016-01-25 04:40:53]

newt@ can you please take another look? The fallback icon is now generated in
Javascript.

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

newt@chromium.org changed reviewers:
+ dbeam@chromium.org

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

dbeam should take a look. He's much more familiar with the history webui page.

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/history/history.js (right):

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
chrome/browser/resources/history/history.js:491: // generated in Javascript
because Android does not yet support text
Maybe: "an HTML/CSS fallback favicon is generated"? "generated in JavaScript"
makes me think that someone has written a rendering engine in JavaScript.

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/large_icon_source.cc (right):

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/large_icon_source.cc:113: // RenderFallbackIconBitmap()
cannot draw fallback icons on Android. See
Instead of sending a 1x1 bitmap, could you send the fallback style (color and
letter) encoded in json?

[pkotwicz, 2016-01-26 01:22:05]

newt@ and dbeam@: I was wondering if I can get some advice on implementing
newt@'s suggestion.

My understanding is that newt@ is suggesting that history.js should request
favicons on Android via chrome.send('getLargeIcon', url) and that
BrowsingHistoryHandler should do all of the processing.

In order for BrowsingHistoryHandler::HandleGetLargeIcon() to be able to call
back to history.js, JavaScript must pass an id which uniquely identifies visits
to native. I cannot use Visit.id_ because HistoryModel.nextVisitId_ may get
reset. Should I introduce a new id to the Visit class or is there a better way?

[pkotwicz, 2016-01-27 16:01:51]

dbeam@ Ping!

[Dan Beam, 2016-01-28 04:03:37]

out of curiosity, could we just return the letter/color encoded in the bitmap
instead?

[pkotwicz, 2016-01-28 04:14:06]

dbeam@: How would I get the bitmap bytes from Javascript?

[Dan Beam, 2016-01-28 04:29:43]

On 2016/01/28 04:14:06, pkotwicz wrote:
> dbeam@: How would I get the bitmap bytes from Javascript?

I mean, instead of doing this odd dance where you ask the C++ for something, and
if it gives you back something empty-ish and you show text, you just paint some
text into the bitmap and respond with that (and don't touch the history
css/html/js, as it's being rewritten anyways).

does that make sense?  just paint the image in a canvas in C++ and rasterize
before returning?  is that feasible?

[pkotwicz, 2016-01-28 16:14:56]

dbeam@: The issue that I am trying to get around is that painting text into a
canvas in C++ on Android is unsupported. Android includes
canvas_notimplemented.cc which stubs out the text drawing code. I am unaware of
anyone working on C++ text drawing on Android.

Patch set #1 delegates to Java to draw text to the bitmap but newt@ did not like
that approach

[pkotwicz, 2016-02-05 20:36:06]

Ping!

[pkotwicz, 2016-02-05 20:36:21]

dbeam: Ping!

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

ok, after our talk:

* the current CSS/HTML/JS impl would is not likely to be replaced on mobile yet
as Polymer would add a lot to Android's installer/on-disk size and may suffer
from runtime performance issues

* it makes sense to transform the current desktop+mobile version into an
Android-specific version soon-ish, and we've partially volunteered you for this
work :P

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/history/history.css (left):

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
chrome/browser/resources/history/history.css:469: }
how can you nuke all this without breaking something?  are all of these using
.favicon more generically now?

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/history/history.js (right):

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
chrome/browser/resources/history/history.js:60: *           
fallbackFaviconText: string,
is this ever more than a character?

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
chrome/browser/resources/history/history.js:492: // drawing in native.
well, that's not really true... 
https://developer.mozilla.org/en-US/docs/Web/API/Canvas_API/Tutorial/Drawing_...

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
chrome/browser/resources/history/history.js:497: img.onload =
this.onLargeFaviconLoadedAndroid_.bind(this, faviconDiv, img);
you probably don't need to bind img because you can use e.target or
e.currentTarget from the handler

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
chrome/browser/resources/history/history.js:497: img.onload =
this.onLargeFaviconLoadedAndroid_.bind(this, faviconDiv, img);
can we just trigger onerror in the case we fail to load a favicon?

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
chrome/browser/resources/history/history.js:515: if (loadedImg.width == 1 &&
loadedImg.height == 1) {
is it likely that width == 1 and height != 1?

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
chrome/browser/resources/history/history.js:516:
faviconDiv.classList.add('fallback-favicon');
can we just create a wrapper div *if* the icon fails to load?

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
chrome/browser/resources/history/history.js:517: faviconDiv.innerHTML =
this.fallbackFaviconText_;
can you use textContent instead?  (innerHTML is bad for security in general)

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
chrome/browser/resources/history/history.js:1387:
HistoryView.prototype.getGroupedVisitsDOM_ = function(
ah, you did check the grouped view :) sorry for not noticing you updated this
code already

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
chrome/browser/resources/history/history.js:1413:
createElementWithClassName('div', 'favicon'));
is it necessary to add this wrapper on desktop?

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/large_icon_source.cc (right):

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/large_icon_source.cc:113: // RenderFallbackIconBitmap()
cannot draw fallback icons on Android. See
On 2016/01/25 19:43:35, newt (OOO until 2-8) wrote:
> Instead of sending a 1x1 bitmap, could you send the fallback style (color and
> letter) encoded in json?

how would it be read?

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/large_icon_source.cc:117: bitmap.allocN32Pixels(1, 1);
why not a 0x0?

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

dbeam@ can you please take another look? I think that I have addressed all of
your comments

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/history/history.css (left):

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
chrome/browser/resources/history/history.css:469: }
The rtl background-position-x CSS switches the position of the icon for right to
left languages.

The wrapper div gives us the "changing the position of the icon" behavior for
free.
The icon is stretched to cover the entire ".favicon wrapper div" so it is not
necessary to specify a background-position-x within the div.

.favicon CSS rule contains all of the CSS rules on lines 460 - 462

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/history/history.js (right):

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
chrome/browser/resources/history/history.js:60: *           
fallbackFaviconText: string,
Yes, fallbackFaviconText is "IP" for IP urls. Otherwise, this always a single
character

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
chrome/browser/resources/history/history.js:491: // generated in Javascript
because Android does not yet support text
On 2016/01/25 19:43:35, newt (OOO until 2-8) wrote:
> Maybe: "an HTML/CSS fallback favicon is generated"? "generated in JavaScript"
> makes me think that someone has written a rendering engine in JavaScript.

Done.

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
chrome/browser/resources/history/history.js:492: // drawing in native.
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++)

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
chrome/browser/resources/history/history.js:497: img.onload =
this.onLargeFaviconLoadedAndroid_.bind(this, faviconDiv, img);
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.

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
chrome/browser/resources/history/history.js:497: img.onload =
this.onLargeFaviconLoadedAndroid_.bind(this, faviconDiv, img);
On 2016/02/05 20:42:11, Dan Beam wrote:
> you probably don't need to bind img because you can use e.target or
> e.currentTarget from the handler

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
chrome/browser/resources/history/history.js:515: if (loadedImg.width == 1 &&
loadedImg.height == 1) {
It is not likely. I changed the if() to check just the width.

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
chrome/browser/resources/history/history.js:516:
faviconDiv.classList.add('fallback-favicon');
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?

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/resource...
chrome/browser/resources/history/history.js:517: faviconDiv.innerHTML =
this.fallbackFaviconText_;
On 2016/02/05 20:42:11, Dan Beam wrote:
> can you use textContent instead?  (innerHTML is bad for security in general)

Done.

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/large_icon_source.cc (right):

https://codereview.chromium.org/1596273003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/large_icon_source.cc:117: bitmap.allocN32Pixels(1, 1);
See my other comment about sending the dominant color of the bitmap back to
JavaScript.

[Dan Beam, 2016-02-06 03:34:29]

pre-lgtm but get newt@'s consensus, IMO

https://codereview.chromium.org/1596273003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/history/externs.js (right):

https://codereview.chromium.org/1596273003/diff/40001/chrome/browser/resource...
chrome/browser/resources/history/externs.js:22: *           
fallbackFaviconChar: string,
i don't think this is right, you should run the closure compiler:
https://chromium.googlesource.com/chromium/src/+/master/docs/closure_compilat...

https://codereview.chromium.org/1596273003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/history/history_mobile.css (right):

https://codereview.chromium.org/1596273003/diff/40001/chrome/browser/resource...
chrome/browser/resources/history/history_mobile.css:224: background-size: 32px;
i now understand what you mean: you're returning the dominant color and
stretching it to 32x32. sneaky. as long as this is fast enough, I suppose I'm OK
with this, but I understand newt@'s apprehension now...

for what it's worth: you can probably paint the image's content onto a canvas
and scrape the pixel at (0,0), but dunno if that's really better

https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/dra...
https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/get...

[pkotwicz, 2016-02-06 04:31:27]

newt@ can you please take a look?

https://codereview.chromium.org/1596273003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/history/externs.js (right):

https://codereview.chromium.org/1596273003/diff/40001/chrome/browser/resource...
chrome/browser/resources/history/externs.js:22: *           
fallbackFaviconChar: string,
Thanks for catching!

[newt (away), 2016-02-08 18:28:19]

lgtm

Sending the fallback color via a 1x1 icon is a bit hacky, but it looks like
there's not a simple alternative. As long as the current approach is performant,
I'm fine with it.

[pkotwicz, 2016-02-08 21:40:55]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-02-08 21:40:56]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1596273003/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-02-08 21:42:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1596273003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1596273003/60001

[commit-bot: I haz the power, 2016-02-09 01:07:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-09 01:08:05]

Description was changed from

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

This CL makes the history page use a rounded rectangle with the first letter of
the URL for history entries for which there is no large enough favicon.
Previously, the history page did a really bad resize for entries for which there
is no large enough favicon.

This CL:
- Introduces an Android version of FallbackIconService:
FallbackIconServiceImplAndroid which uses RoundedIconGenerator
- Encodes the SkBitmap produced by FallbackIconService to PNG on a worker
  thread. Encoding to PNG takes on average 5ms on a Nexus 5.

BUG=566012
==========

to

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

This CL makes the history page use a rounded rectangle with the first letter of
the URL for history entries for which there is no large enough favicon.
Previously, the history page did a really bad resize for entries for which there
is no large enough favicon.

This CL:
- Introduces an Android version of FallbackIconService:
FallbackIconServiceImplAndroid which uses RoundedIconGenerator
- Encodes the SkBitmap produced by FallbackIconService to PNG on a worker
  thread. Encoding to PNG takes on average 5ms on a Nexus 5.

BUG=566012

Committed: https://crrev.com/52ce2121706ad270ecf5ee01b400e3f0aabd941b
Cr-Commit-Position: refs/heads/master@{#374247}
==========

[commit-bot: I haz the power, 2016-02-09 01:08:07]

Patchset 4 (id:??) landed as
https://crrev.com/52ce2121706ad270ecf5ee01b400e3f0aabd941b
Cr-Commit-Position: refs/heads/master@{#374247}