843234 - Default bookmarks creation should be async

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

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.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

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.

Comment 2

[: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.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch

> 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.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch v3

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!

Comment 5

[:Margaret Leibovic]

Comment on attachment 716791 [details] [diff] [review]
Patch v3

I think this is the wrong patch...

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch v3

Doh.

Comment 7

[: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.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch

Thanks for being thorough.

Comment 9

[: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.

Comment 10

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c34d5490c0ad

Comment 11

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/c34d5490c0ad