[Athena] Hack to display favicons for web activities in overview mode

[Athena] Hack to display favicons for web activities in overview mode.

BUG=411631
TEST=None
TBR=oshima,sadrul (TBR for changes to athena/content/DEPS)

Committed: https://crrev.com/897d04289e23b3d02ff4de08b9c327c3bb574324
Cr-Commit-Position: refs/heads/master@{#294400}

[pkotwicz, 2014-09-06 19:01:05]

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

[pkotwicz, 2014-09-06 19:01:06]

Oshima, PTAL

With respect to the position of the title in overview mode, it is weird for the
title to be positioned as if there was an icon even if the activity has no icon.
I think that things will look good once we have default icons

[sadrul, 2014-09-07 05:39:18]

Cool! I assume this works for both athena_main and chrome with use_athena=1?

https://codereview.chromium.org/550643002/diff/1/athena/activity/activity_fra...
File athena/activity/activity_frame_view.cc (left):

https://codereview.chromium.org/550643002/diff/1/athena/activity/activity_fra...
athena/activity/activity_frame_view.cc:114: SchedulePaint();
You need to keep this here. See https://codereview.chromium.org/549593002/

https://codereview.chromium.org/550643002/diff/1/athena/content/web_activity.cc
File athena/content/web_activity.cc (right):

https://codereview.chromium.org/550643002/diff/1/athena/content/web_activity....
athena/content/web_activity.cc:509: base::Unretained(this)));
Use a weak-ptr here instead.

https://codereview.chromium.org/550643002/diff/1/athena/content/web_activity....
athena/content/web_activity.cc:521: icon_ = CreateFaviconImageSkia(
You should probably look at |http_status_code| before doing anything?

[pkotwicz, 2014-09-08 21:36:18]

Oshima, can you please take another look?

This works both in chrome with use_athena and in athena_main.

However, we eventually want to use FaviconService and FaviconTabHelper because
we will want to get favicons at other times than when navigating to a page (e.g.
some incarnation of bookmarks)

https://codereview.chromium.org/550643002/diff/1/athena/activity/activity_fra...
File athena/activity/activity_frame_view.cc (left):

https://codereview.chromium.org/550643002/diff/1/athena/activity/activity_fra...
athena/activity/activity_frame_view.cc:114: SchedulePaint();
You're right. Thanks for catching this!

https://codereview.chromium.org/550643002/diff/1/athena/content/web_activity.cc
File athena/content/web_activity.cc (right):

https://codereview.chromium.org/550643002/diff/1/athena/content/web_activity....
athena/content/web_activity.cc:509: base::Unretained(this)));
On 2014/09/07 05:39:18, sadrul wrote:
> Use a weak-ptr here instead.

Done.

https://codereview.chromium.org/550643002/diff/1/athena/content/web_activity....
athena/content/web_activity.cc:521: icon_ = CreateFaviconImageSkia(
According to MultiResolutionImageResourceFetcher::OnURLFetchComplete() |bitmaps|
should be empty if |http_status_code| != 200

[oshima, 2014-09-08 21:53:25]

lgtm

[sadrul, 2014-09-09 22:52:40]

https://codereview.chromium.org/550643002/diff/20001/athena/content/web_activ...
File athena/content/web_activity.cc (right):

https://codereview.chromium.org/550643002/diff/20001/athena/content/web_activ...
athena/content/web_activity.cc:503: // (crbug.com/401997)
You may want to weak_ptr_factory_.InvalidateWeakPtrs() here (And in
DidNavigateMainFrame()) so that in case there's a pending download, it doesn't
actually trigger OnDidDownloadFavicon() and change the icon incorrectly.

[pkotwicz, 2014-09-10 00:40:39]

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

[pkotwicz, 2014-09-10 00:40:40]

Sadrul, can you please take another look?

I have added WebActivity::icon_url_ in order to detect if
WebActivity::OnDidDownloadFavicon() if for a stale request.

Just learned about this: In the places that it can be used,
base::CancelableCallback seems like a better choice than using
base::WeakPtrFactory to get a callback which can be canceled.

[sadrul, 2014-09-10 01:03:38]

On 2014/09/10 00:40:40, pkotwicz wrote:
> Sadrul, can you please take another look?
> 
> I have added WebActivity::icon_url_ in order to detect if
> WebActivity::OnDidDownloadFavicon() if for a stale request.
> 
> Just learned about this: In the places that it can be used,
> base::CancelableCallback seems like a better choice than using
> base::WeakPtrFactory to get a callback which can be canceled.

Why add one more member variable we need to maintain (icon_url_, or the
cancelable task) instead of using the one we already have (weak_ptr_factory_)?

[pkotwicz, 2014-09-10 01:21:35]

I think that it is unclear that the code relies on being the sole user of the
weak_ptr_factory_

w.r.t to base::CancellableCallback, if I used it I would not need to use
|weak_ptr_factory_|. The reason that I cannot use it is that there is no
base::CancellableCallback with the number of parameters that I need.

[sadrul, 2014-09-10 01:31:19]

On 2014/09/10 01:21:35, pkotwicz wrote:
> I think that it is unclear that the code relies on being the sole user of the
> weak_ptr_factory_

I do not understand what you mean here.

We use invalidating weak-ptr for this kind of thing elsewhere, so it should work
here too. If it doesn't, then the code is doing something wrong.

> 
> w.r.t to base::CancellableCallback, if I used it I would not need to use
> |weak_ptr_factory_|. The reason that I cannot use it is that there is no
> base::CancellableCallback with the number of parameters that I need.

[pkotwicz, 2014-09-10 02:07:01]

You're right, a lot of code uses invalidating a WeakPtrFactory for this type of
thing. Changed

[sadrul, 2014-09-11 01:03:17]

LGTM

https://codereview.chromium.org/550643002/diff/60001/athena/content/web_activ...
File athena/content/web_activity.cc (right):

https://codereview.chromium.org/550643002/diff/60001/athena/content/web_activ...
athena/content/web_activity.cc:508: weak_ptr_factory_.InvalidateWeakPtrs();
Should we invalidate regardless of whether we have a valid candidate favicon?

https://codereview.chromium.org/550643002/diff/60001/athena/content/web_activ...
athena/content/web_activity.cc:526: if (icon_url_ != url) {
Remove.

https://codereview.chromium.org/550643002/diff/60001/athena/content/web_activ...
File athena/content/web_activity.h (right):

https://codereview.chromium.org/550643002/diff/60001/athena/content/web_activ...
athena/content/web_activity.h:108: GURL icon_url_;
Remove

[pkotwicz, 2014-09-11 14:57:30]

Patchset #5 (id:80001) has been deleted

[pkotwicz, 2014-09-11 15:01:44]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2014-09-11 15:01:51]

The CQ bit was unchecked by pkotwicz@chromium.org

[pkotwicz, 2014-09-11 15:02:34]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-09-11 15:03:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/550643002/100001

[pkotwicz, 2014-09-11 15:04:01]

The CQ bit was unchecked by pkotwicz@chromium.org

[pkotwicz, 2014-09-11 15:05:02]

Patchset #5 (id:100001) has been deleted

[pkotwicz, 2014-09-11 15:20:45]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-09-11 15:21:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/550643002/140001

[commit-bot: I haz the power, 2014-09-11 16:25:01]

Committed patchset #6 (id:140001) as 601cffe75203623015062a8a64e42c16750b58e2

[commit-bot: I haz the power, 2014-09-11 16:29:54]

Patchset 6 (id:??) landed as
https://crrev.com/897d04289e23b3d02ff4de08b9c327c3bb574324
Cr-Commit-Position: refs/heads/master@{#294400}