Last Comment Bug 699786 - Cancel pending favicon requests for tabs that changed location
: Cancel pending favicon requests for tabs that changed location
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
: P3 normal (vote)
: ---
Assigned To: Lucas Rocha (:lucasr)
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on: 997478
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-04 07:27 PDT by Lucas Rocha (:lucasr)
Modified: 2014-04-16 16:08 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
(1/3) Use Android's AsyncTask in Favicons (4.16 KB, patch)
2011-11-17 05:31 PST, Lucas Rocha (:lucasr)
blassey.bugs: review+
Details | Diff | Splinter Review
(2/3) Add API and infra to support favicon load cancellation (6.76 KB, patch)
2011-11-17 05:32 PST, Lucas Rocha (:lucasr)
blassey.bugs: review+
Details | Diff | Splinter Review
(3/3) Cancel pending favicon loads when location changes (4.87 KB, patch)
2011-11-17 05:33 PST, Lucas Rocha (:lucasr)
blassey.bugs: review+
Details | Diff | Splinter Review

Description Lucas Rocha (:lucasr) 2011-11-04 07:27:30 PDT
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.
Comment 1 Lucas Rocha (:lucasr) 2011-11-17 05:31:29 PST
Created attachment 575150 [details] [diff] [review]
(1/3) Use Android's AsyncTask in Favicons
Comment 2 Lucas Rocha (:lucasr) 2011-11-17 05:32:33 PST
Created attachment 575151 [details] [diff] [review]
(2/3) Add API and infra to support favicon load cancellation
Comment 3 Lucas Rocha (:lucasr) 2011-11-17 05:33:10 PST
Created attachment 575152 [details] [diff] [review]
(3/3) Cancel pending favicon loads when location changes
Comment 4 Brad Lassey [:blassey] (use needinfo?) 2011-11-17 08:42:06 PST
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
Comment 5 Lucas Rocha (:lucasr) 2011-11-17 08:46:27 PST
(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 6 Brad Lassey [:blassey] (use needinfo?) 2011-11-17 13:08:27 PST
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)
Comment 7 Lucas Rocha (:lucasr) 2011-11-17 13:09:58 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.