Last Comment Bug 695835 - Move favicon download to a separate thread
: Move favicon download to a separate thread
Status: RESOLVED FIXED
[QA?]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P1 normal (vote)
: ---
Assigned To: Gian-Carlo Pascutto [:gcp]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-19 13:27 PDT by Mark Finkle (:mfinkle) (use needinfo?)
Modified: 2012-01-09 11:32 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Patch. Move favicon download to AsyncTask (2.25 KB, patch)
2011-10-20 10:37 PDT, Gian-Carlo Pascutto [:gcp]
mark.finkle: review+
Details | Diff | Splinter Review

Description Mark Finkle (:mfinkle) (use needinfo?) 2011-10-19 13:27:48 PDT
Currently we download on the main UI thread. The download takes up too much time.
Comment 1 Gian-Carlo Pascutto [:gcp] 2011-10-20 10:37:09 PDT
Created attachment 568438 [details] [diff] [review]
Patch. Move favicon download to AsyncTask
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-20 17:53:22 PDT
Comment on attachment 568438 [details] [diff] [review]
Patch. Move favicon download to AsyncTask

         }
>+        }
>         if (rel.indexOf("icon") != -1) {
>-            mMainHandler.post(new Runnable() { 

Great! Just add a blank line between the class and the "if"
Comment 3 [:fabrice] Fabrice Desré 2011-10-20 20:28:58 PDT
(In reply to Mark Finkle (:mfinkle) from comment #2)
> Comment on attachment 568438 [details] [diff] [review] [diff] [details] [review]
> Patch. Move favicon download to AsyncTask
> 
>          }
> >+        }
> >         if (rel.indexOf("icon") != -1) {
> >-            mMainHandler.post(new Runnable() { 

rel.indexOf("icon") is not a very good test. It will match for <link rel="rubicon"> for instance.
There's a proper algorithm described here:http://www.whatwg.org/specs/web-apps/current-work/multipage/links.html#rel-icon

We can also support apple-touch-icon like we did at http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/browser-ui.js#1027
Comment 4 Brad Lassey [:blassey] (use needinfo?) 2011-10-20 23:04:33 PDT
Comment on attachment 568438 [details] [diff] [review]
Patch. Move favicon download to AsyncTask

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

::: embedding/android/GeckoApp.java
@@ +517,2 @@
>          if (rel.indexOf("icon") != -1) {
> +            mMainHandler.post(new Runnable() {

why are you posting this to the main thread?
Comment 5 Gian-Carlo Pascutto [:gcp] 2011-10-21 10:01:56 PDT
Incorporated mfinkle and blassey's comment. Fabrice, fixing the actual functionality should prolly go into a seperate bug.

http://hg.mozilla.org/projects/birch/rev/1685afdada81

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