Closed Bug 843234 Opened 9 years ago Closed 9 years ago

Default bookmarks creation should be async

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Linux
defect
Not set
normal

Tracking

(fennec+)

RESOLVED FIXED
Firefox 22
Tracking Status
fennec + ---

People

(Reporter: wesj, Assigned: wesj)

References

Details

(Keywords: perf)

Attachments

(1 file, 4 obsolete files)

The startup problem with about:home we ran into (bug 835356) was due to a slow path in GeckoJarReader being hit when we create the default bookmarks. I don't know that having the default bookmarks favicons ready instantly is that important. We should do it in a separate AsyncTask.
tracking-fennec: --- → ?
Keywords: perf
Hardware: x86 → ARM
Attached patch Patch (obsolete) — Splinter Review
This moves just the favicon creation to its own task. I tried moving the entire bookmark creation to its own task, which does feel a bit faster to me. But to make that work I had to add a bit so that about:home starts up showing empty thumbnails and then fills them in slightly after. It felt kinda weird, so I went this way instead, but we could play with the other.
Attachment #716163 - Flags: review?(margaret.leibovic)
Comment on attachment 716163 [details] [diff] [review]
Patch

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

I basically think this is good, but I'd like to see another version of this before we land it.

::: mobile/android/base/db/BrowserProvider.java.in
@@ +1024,5 @@
> +                    final String url = getLocalizedProperty(bookmark, "url", locale);
> +                    createBookmark(db, title, url, pos);
> +
> +                    // create icons in a separate task to avoid blocking about:home on startup
> +                    (new GeckoAsyncTask<Void, Void, Void>(GeckoApp.mAppContext, GeckoAppShell.getHandler()) {

Let's try using mContext instead of GeckoApp.mAppContext (this would get rid of the dependency on GeckoApp).

@@ +1030,5 @@
> +                        public Void doInBackground(Void... params) {
> +                            try {
> +                                Bitmap icon = null;
> +                                // Look for an optional icon data URI
> +                                if (bookmark.has("icon")) {

This check is pretty cheap, maybe you should check this first and avoid the GeckoAsyncTask altogether if there's no icon.

@@ +1038,5 @@
> +                                createFavicon(db, url, icon);
> +                            } catch (JSONException e) {
> +                                Log.e(LOGTAG, "Error creating distribution bookmark icon", e);
> +                            }
> +                            return null;

Is there a reason you need to explicitly return null here, or can you omit this?

@@ +1071,5 @@
> +                    final String url = mContext.getString(urlId);
> +                    createBookmark(db, title, url, pos);
> +
> +                    // create icons in a separate task to avoid blocking about:home on startup
> +                    (new GeckoAsyncTask<Void, Void, Void>(GeckoApp.mAppContext, GeckoAppShell.getHandler()) {

Same thing here.

@@ +1079,5 @@
> +                            if (icon == null) {
> +                                icon = getDefaultFaviconFromDrawable(name);
> +                            }
> +                            createFavicon(db, url, icon);
> +                            return null;

Same thing here.

@@ +1081,5 @@
> +                            }
> +                            createFavicon(db, url, icon);
> +                            return null;
> +                        }
> +                    }).execute();

I wonder if there's a way to factor these GeckoAsyncTasks into a shared method. But I suppose the logic to get the icon is different enough (and that's what you want on the background thread), that it isn't worth it. If only we could pass functions in Java!

@@ +1111,4 @@
>              // Return early if there's no icon to set
>              if (icon == null) {
>                  return;
>              }

Since you're factoring this out, we can just avoid calling createFavicon if the icon is null instead of bailing out at the beginning of the method.
Attachment #716163 - Flags: review?(margaret.leibovic) → review-
tracking-fennec: ? → +
Attached patch Patch (obsolete) — Splinter Review
> Let's try using mContext instead of GeckoApp.mAppContext (this would get rid
> of the dependency on GeckoApp).

In order to not have to do something ugly like access ((GeckoApp)mContext).getHandler(), I just made it so that UIAsyncTask can take a null UI thread.

> Is there a reason you need to explicitly return null here, or can you omit
> this?
Async task requires you to declare a return type, Void in this case. If you don't return something you'll get compile errors.
Assignee: nobody → wjohnston
Attachment #716163 - Attachment is obsolete: true
Attachment #716780 - Flags: review?(margaret.leibovic)
Attached patch Patch v3 (obsolete) — Splinter Review
Margaret reminded me that I don't need to use UIAsyncTask if I don't need a callback when the task is done. I can just post to the background thread!
Attachment #716780 - Attachment is obsolete: true
Attachment #716780 - Flags: review?(margaret.leibovic)
Attachment #716791 - Flags: review?(margaret.leibovic)
Comment on attachment 716791 [details] [diff] [review]
Patch v3

I think this is the wrong patch...
Attached patch Patch v3 (obsolete) — Splinter Review
Doh.
Attachment #716791 - Attachment is obsolete: true
Attachment #716791 - Flags: review?(margaret.leibovic)
Attachment #716818 - Flags: review?(margaret.leibovic)
Comment on attachment 716818 [details] [diff] [review]
Patch v3

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

Almost there. r- for the s/return/continue.

If you feel like being thorough, you could use bug 836450 comment 5 to test the distribution bookmarks, since we don't have robocop tests for that yet :)

::: mobile/android/base/db/BrowserProvider.java.in
@@ +20,5 @@
>  import java.util.regex.Pattern;
>  import java.util.regex.Matcher;
>  
>  import org.mozilla.gecko.Distribution;
> +import org.mozilla.gecko.GeckoAppShell;

This isn't needed anymore.

@@ +64,5 @@
>  import android.graphics.Bitmap;
>  import android.graphics.drawable.BitmapDrawable;
>  import android.net.Uri;
>  import android.os.Build;
> +import android.os.Handler;

Do you need this, too?

@@ +1025,5 @@
>                      pos++;
> +
> +                    // return early if there is no icon for this bookmark
> +                    if (!bookmark.has("icon"))
> +                        return pos;

This is in a for loop, so this should be a continue. (Also, I think we like bracing single-line ifs in this file.)

@@ +1033,5 @@
> +                        public void run() {
> +                            try {
> +                                Bitmap icon = null;
> +                                String iconData = bookmark.getString("icon");
> +                                icon = BitmapUtils.getBitmapFromDataURI(iconData);

This is probably just left over from moving code around, but there's no reason to declare icon = null above.

@@ +1078,5 @@
> +                            if (icon == null) {
> +                                icon = getDefaultFaviconFromDrawable(name);
> +                            }
> +                            if (icon != null)
> +                                createFavicon(db, url, icon);

Nit: Let's be consistent about bracing single-line ifs.
Attachment #716818 - Flags: review?(margaret.leibovic) → review-
Attached patch PatchSplinter Review
Thanks for being thorough.
Attachment #716818 - Attachment is obsolete: true
Attachment #717272 - Flags: review?(margaret.leibovic)
Comment on attachment 717272 [details] [diff] [review]
Patch

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

Nice.

::: mobile/android/base/db/BrowserProvider.java.in
@@ +1028,3 @@
>                      }
>  
> +                    // create icons in a separate task to avoid blocking about:home on startup

Nit: s/in a separate task/on a separate thread/

@@ +1068,5 @@
>                      int urlId = urlField.getInt(null);
> +                    final String url = mContext.getString(urlId);
> +                    createBookmark(db, title, url, pos);
> +
> +                    // create icons in a separate task to avoid blocking about:home on startup

Same thing here.
Attachment #717272 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/c34d5490c0ad
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Depends on: 844895
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.