The default bug view has changed. See this FAQ.

Cancel pending favicon requests for tabs that changed location

RESOLVED FIXED

Status

()

Firefox for Android
General
P3
normal
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: lucasr, Assigned: lucasr)

Tracking

unspecified
All
Android
Points:
---

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

Attachments

(3 attachments)

(Assignee)

Description

6 years ago
If favicon is requested but the corresponding tab changes location, we should simply cancel the pending request as we're not interested in its results anymore.
(Assignee)

Updated

6 years ago
OS: Linux → Android
Hardware: x86 → All
Assignee: nobody → lucasr.at.mozilla
Priority: -- → P3
(Assignee)

Comment 1

5 years ago
Created attachment 575150 [details] [diff] [review]
(1/3) Use Android's AsyncTask in Favicons
Attachment #575150 - Flags: review?(blassey.bugs)
(Assignee)

Comment 2

5 years ago
Created attachment 575151 [details] [diff] [review]
(2/3) Add API and infra to support favicon load cancellation
Attachment #575151 - Flags: review?(blassey.bugs)
(Assignee)

Comment 3

5 years ago
Created attachment 575152 [details] [diff] [review]
(3/3) Cancel pending favicon loads when location changes
Attachment #575152 - Flags: review?(blassey.bugs)
Attachment #575150 - Flags: review?(blassey.bugs) → review+
Comment on attachment 575151 [details] [diff] [review]
(2/3) Add API and infra to support favicon load cancellation

Review of attachment 575151 [details] [diff] [review]:
-----------------------------------------------------------------

Nit, make mNextFaviconLoadId a static member of the LoadFaviconTask class
Attachment #575151 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 5

5 years ago
(In reply to Brad Lassey [:blassey] from comment #4)
> Comment on attachment 575151 [details] [diff] [review] [diff] [details] [review]
> (2/3) Add API and infra to support favicon load cancellation
> 
> Review of attachment 575151 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Nit, make mNextFaviconLoadId a static member of the LoadFaviconTask class

Tried that but inner classes can't have static members...
Comment on attachment 575152 [details] [diff] [review]
(3/3) Cancel pending favicon loads when location changes

Review of attachment 575152 [details] [diff] [review]:
-----------------------------------------------------------------

::: embedding/android/GeckoApp.java
@@ +589,5 @@
>          tab.removeTransientDoorHangers();
>  
> +        // Cancel any pending favicon load task
> +        long faviconLoadId = tab.getFaviconLoadId();
> +        if (faviconLoadId > 0)

should this be:?
if (faviconLoadId != Favicons.NOT_LOADING)
Attachment #575152 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 7

5 years ago
(In reply to Brad Lassey [:blassey] from comment #6)
> Comment on attachment 575152 [details] [diff] [review] [diff] [details] [review]
> (3/3) Cancel pending favicon loads when location changes
> 
> Review of attachment 575152 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: embedding/android/GeckoApp.java
> @@ +589,5 @@
> >          tab.removeTransientDoorHangers();
> >  
> > +        // Cancel any pending favicon load task
> > +        long faviconLoadId = tab.getFaviconLoadId();
> > +        if (faviconLoadId > 0)
> 
> should this be:?
> if (faviconLoadId != Favicons.NOT_LOADING)

Yep, fixed.
(Assignee)

Comment 8

5 years ago
Pushed:
http://hg.mozilla.org/projects/birch/rev/fa4ef7d24a26
http://hg.mozilla.org/projects/birch/rev/e20ccbf697a1
http://hg.mozilla.org/projects/birch/rev/9999a423d8ab
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
tracking-fennec: --- → 11+
status-firefox11: --- → fixed
Depends on: 997478
You need to log in before you can comment on or make changes to this bug.