Refactor TaskManager's favicon retrieval approach

Refactor TaskManager's favicon retrieval approach

Instead of listening to unreliable WebContentsObserver events, we now
make use of the FaviconDriverObserver.

R=nick@chromium.org
BUG=530486, 528924
Committed: https://crrev.com/21a49151acdbe9daa4754ee0b974be39cb23a95b
Cr-Commit-Position: refs/heads/master@{#348773}

[afakhry, 2015-09-11 19:55:09]

Hey Nick, please review! I tried it with www.favicon.cc and it's working
perfectly. It also fixes the relevant bug.

[ncarter (slow), 2015-09-11 20:08:45]

This is pretty great!

lgtm but see if you can find a FaviconDriver expert from git history, to make
sure we're using things right.

https://codereview.chromium.org/1338023002/diff/1/chrome/browser/task_managem...
File chrome/browser/task_management/providers/web_contents/renderer_task.cc
(right):

https://codereview.chromium.org/1338023002/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/providers/web_contents/renderer_task.cc:156: if
(icon_url_changed)
What does it mean if this is called but |icon_url_changed| is false?

[afakhry, 2015-09-11 23:02:09]

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

[afakhry, 2015-09-11 23:02:10]

Hey pkotwicz@, I see that you're an owner of favicon/. Could you please take a
look at how we're implementing FaviconDriverObserver and confirm that we're
doing the right thing? Thanks!

[pkotwicz, 2015-09-11 23:51:33]

Thank you for asking me for my feedback!

https://codereview.chromium.org/1338023002/diff/1/chrome/browser/task_managem...
File chrome/browser/task_management/providers/web_contents/renderer_task.cc
(right):

https://codereview.chromium.org/1338023002/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/providers/web_contents/renderer_task.cc:151: //
not necessarily updated.
Can you remove this comment? OnFaviconAvailable() has a super confusing name. It
is called whenever OnFaviconUpdated() is called on the desktop.

On Android, this is called when a apple-touch-icon is available in addition to
when a regular favicon is available. OnFaviconUpdated() is not called when a
apple-touch-icon is available.

https://codereview.chromium.org/1338023002/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/providers/web_contents/renderer_task.cc:156: if
(icon_url_changed)
You actually want to call UpdateFavicon() even if |icon_url_changed| is false.
This will be called with |icon_url_changed| == false if the favicon in the
database is expired and we have just finished downloading a new one.

The favicon pixels may be different. http://www.google.com/favicon.ico is
different today than it was a month ago

[afakhry, 2015-09-12 00:42:21]

Thanks, peter!

https://codereview.chromium.org/1338023002/diff/1/chrome/browser/task_managem...
File chrome/browser/task_management/providers/web_contents/renderer_task.cc
(right):

https://codereview.chromium.org/1338023002/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/providers/web_contents/renderer_task.cc:151: //
not necessarily updated.
On 2015/09/11 23:51:33, pkotwicz wrote:
> Can you remove this comment? OnFaviconAvailable() has a super confusing name.
It
> is called whenever OnFaviconUpdated() is called on the desktop.
> 
> On Android, this is called when a apple-touch-icon is available in addition to
> when a regular favicon is available. OnFaviconUpdated() is not called when a
> apple-touch-icon is available.

I removed the comment. But I have to say I don't know what "apple-touch-icon"
is. I want to confirm however that it's safe to ignore this icon.

https://codereview.chromium.org/1338023002/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/providers/web_contents/renderer_task.cc:156: if
(icon_url_changed)
On 2015/09/11 23:51:33, pkotwicz wrote:
> You actually want to call UpdateFavicon() even if |icon_url_changed| is false.
> This will be called with |icon_url_changed| == false if the favicon in the
> database is expired and we have just finished downloading a new one.
> 
> The favicon pixels may be different. http://www.google.com/favicon.ico is
> different today than it was a month ago

Thanks for the clarification. Done!

[pkotwicz, 2015-09-14 14:39:17]

https://codereview.chromium.org/1338023002/diff/1/chrome/browser/task_managem...
File chrome/browser/task_management/providers/web_contents/renderer_task.cc
(right):

https://codereview.chromium.org/1338023002/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/providers/web_contents/renderer_task.cc:151: //
not necessarily updated.
It is safe to ignore

apple-touch-icons were added by Apple to denote favicons with special effects
added. The intention of these icons is to provide a large icon when the web page
is added to the home screen. (You can read more about them at
https://en.wikipedia.org/wiki/Favicon) Chrome only cares about apple-touch-icons
on iOS and Android.

[afakhry, 2015-09-14 16:12:12]

On 2015/09/14 14:39:17, pkotwicz wrote:
>
https://codereview.chromium.org/1338023002/diff/1/chrome/browser/task_managem...
> File chrome/browser/task_management/providers/web_contents/renderer_task.cc
> (right):
> 
>
https://codereview.chromium.org/1338023002/diff/1/chrome/browser/task_managem...
> chrome/browser/task_management/providers/web_contents/renderer_task.cc:151: //
> not necessarily updated.
> It is safe to ignore
> 
> apple-touch-icons were added by Apple to denote favicons with special effects
> added. The intention of these icons is to provide a large icon when the web
page
> is added to the home screen. (You can read more about them at
> https://en.wikipedia.org/wiki/Favicon) Chrome only cares about
apple-touch-icons
> on iOS and Android.

Thanks peter! Does it look good to you now?

[pkotwicz, 2015-09-14 17:41:55]

LGTM

Aside: I think that it is kind of weird that we add the Observer in cases that
RendererTask::OnFaviconUpdated() is a no-op

[afakhry, 2015-09-14 22:33:50]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2015-09-14 22:33:51]

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

[commit-bot: I haz the power, 2015-09-14 22:36:19]

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

[commit-bot: I haz the power, 2015-09-14 23:50:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-14 23:51:13]

Patchset 3 (id:??) landed as
https://crrev.com/21a49151acdbe9daa4754ee0b974be39cb23a95b
Cr-Commit-Position: refs/heads/master@{#348773}

[commit-bot: I haz the power, 2015-09-23 12:39:29]

Patchset 3 (id:??) landed as
https://crrev.com/21a49151acdbe9daa4754ee0b974be39cb23a95b
Cr-Commit-Position: refs/heads/master@{#348773}