Closed Bug 699786 Opened 8 years ago Closed 8 years ago

Cancel pending favicon requests for tabs that changed location

Categories

(Firefox for Android :: General, defect, P3)

All
Android
defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox11 --- fixed
fennec 11+ ---

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

Attachments

(3 files)

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.
OS: Linux → Android
Hardware: x86 → All
Assignee: nobody → lucasr.at.mozilla
Priority: -- → P3
Attachment #575150 - 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+
(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+
(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.
tracking-fennec: --- → 11+
Depends on: 997478
You need to log in before you can comment on or make changes to this bug.