http://people.mozilla.org/~mfinkle/fennec/profiles/nexuss-home-startup-firstrun-20140910.trace This'll be extracting and storing either default favicons or default tiles.

Slightly odd general question: why do we need to do this stuff on first run anyway? Wouldn't it make more sense to compile a pre-initialised database into our binary and just vomit it into the user's profile when required? We'd trade binary size for first-run startup speed. Maybe not worth it... I'll see if I can expedite this operation.

More specifically, the stupidity here is that we're shipping encoded bitmaps which we're loading, decoding, then expensively re-encoding for storage. That's kinda dumb: let's encode them once how we want them and then just copy them straight into the DB. That way, we don't need to use Bitmap.compress at all.

The latter is a reasonable solution. Shipping a pre-packaged DB isn't a good idea -- we don't know what version of sqlite is on the destination device, nor do we know what the localized default bookmarks will be.

Here, I adjust two of the three sources of default favicons to use a more sensible approach. Unfortunately, favicons we're fetching from the Android resource system are probably not susceptible to this optimisation. The use of Reflection in getDefaultFaviconFromDrawable is also questionable, but I'm as yet unsure what logic brought us to this point.

(To clarify, for this to work we need access to the raw bytes of the png (or whatever) being used for the favicon. It must therefore either be placed in /res/raw, or delivered a different way. I'm uncertain what is the preferred approach) Nevertheless, we seem to be using this route rather often :/

Shifted the resources into raw (that turned out to be trivial), eliminated the inefficient pathway, greatly simplified the code (there was some repetition of the evil reflective code), and refactored LoadFaviconTask to share the common IO routine. Neato. Testing shows that the favicons are ending up in the DB and show up in the bookmarks list, so this seems to have worked.

Comment on attachment 8487610 [details] [diff] [review] Don't decode-to-encode default favicons Review of attachment 8487610 [details] [diff] [review]: ----------------------------------------------------------------- Some questions and arrangement nits. ::: mobile/android/base/db/LocalBrowserDB.java @@ +36,5 @@ > import org.mozilla.gecko.db.BrowserContract.SyncColumns; > import org.mozilla.gecko.db.BrowserContract.Thumbnails; > import org.mozilla.gecko.db.BrowserDB.FilterFlags; > import org.mozilla.gecko.distribution.Distribution; > +import org.mozilla.gecko.favicons.LoadFaviconTask; It's unfortunate that we have to take this import just to get the constant. Can we import the constant instead? @@ +196,5 @@ > bookmarkValues.add(bookmarkValue); > > + ConsumedInputStream faviconStream = getDefaultFaviconFromDrawable(context, name); > + if (faviconStream == null) { > + faviconStream = getDefaultFaviconFromPath(context, name); Why did you invert these two cases? @@ +305,5 @@ > try { > final String iconData = bookmark.getString("icon"); > + > + byte[] icon = null; > + final String base64 = iconData.substring(iconData.indexOf(',') + 1); Encapsulate this logic in BitmapUtils, please. getBitmapBytesFromDataURI? ::: mobile/android/base/util/IOUtils.java @@ +56,5 @@ > + * @param bufferSize The initial size of the buffer to allocate. It will be grown as > + * needed, but if the caller knows something about the InputStream then > + * passing a good value here can improve performance. > + */ > + public static ConsumedInputStream readFully(InputStream iStream, int bufferSize) { Gosh I wish we had tests for this.

(In reply to Richard Newman [:rnewman] from comment #9) > @@ +196,5 @@ > > bookmarkValues.add(bookmarkValue); > > > > + ConsumedInputStream faviconStream = getDefaultFaviconFromDrawable(context, name); > > + if (faviconStream == null) { > > + faviconStream = getDefaultFaviconFromPath(context, name); > > Why did you invert these two cases? getDefaultFaviconFromDrawable successfully returns a favicon in this situation more often than getDefaultFaviconFromPath does. (Actually, it seems all our shipped bookmark favicons are provided as drawables for now, so let's do the right thing first). A miniscule performance improvement. > ::: mobile/android/base/util/IOUtils.java > @@ +56,5 @@ > > + * @param bufferSize The initial size of the buffer to allocate. It will be grown as > > + * needed, but if the caller knows something about the InputStream then > > + * passing a good value here can improve performance. > > + */ > > + public static ConsumedInputStream readFully(InputStream iStream, int bufferSize) { > > Gosh I wish we had tests for this. I wish we had a unit test framework that wasn't shit. Having to bolt unit tests for stuff in the main tree onto Sync's JUnit stuff is not really acceptable, particularly because the tests aren't on automation yet. Robocop isn't an acceptable way to write unit tests. It's slightly clunky (but we can live with that) and it has a detrimental effect on our binary size and performance, because optimiser performance is inversely proportional to robocop test coverage. You need to remember to annotate everything you touch to prevent your test from randomly failing based on Proguard's mood, and that of course hurts performance. If we wrote unit tests properly, under that scheme, we'd see virtually no optimisation. Any ideas when JUnit support is going to hit the main tree? It seems the correct approach is to move all tests that arent sync-specific out of the git repo and into m-c, and then start writing unit tests properly.

You can already write m-c only junit tests. There's no coupling to Sync. We already do this for some features, even though they don't run on automation. I suspect that treeherder makes this easier. Check with Nick.

Comment on attachment 8488456 [details] [diff] [review] Don't decode-to-encode default favicons Review of attachment 8488456 [details] [diff] [review]: ----------------------------------------------------------------- If this works, I'm happy with it! ::: mobile/android/base/db/LocalBrowserDB.java @@ +204,4 @@ > continue; > } > > + byte[] icon = faviconStream.getTruncatedData(); This can OOM. Can we make sure we happily continue if it fails?

(In reply to Richard Newman [:rnewman] from comment #13) > Comment on attachment 8488456 [details] [diff] [review] > Don't decode-to-encode default favicons > > Review of attachment 8488456 [details] [diff] [review]: > ----------------------------------------------------------------- > > If this works, I'm happy with it! > > ::: mobile/android/base/db/LocalBrowserDB.java > @@ +204,4 @@ > > continue; > > } > > > > + byte[] icon = faviconStream.getTruncatedData(); > > This can OOM. Can we make sure we happily continue if it fails? Prettymuch *everything* can OOM. It's unclear how to decide when to insulate against it? (bundled icons don't seem like the sort of thing that could erroneously have us allocating ridiculously-sized buffers, after all) I've wrapped this one up in a catch that moves on to the next icon if this one OOMs. This will of course cause the failed icon not to be present. I guess that's preferable to not having a working database.

I'm seeing bits like this in logcat after a fresh install ... it's a |Log.wtf() vs. a Log.i(), so is this expected (warning / safe-to-ignore) ? V/JNIHelp (26310): Registering com/google/android/gms/org/conscrypt/NativeCrypto's 235 native methods... D/dalvikvm(26310): GC_CONCURRENT freed 324K, 4% free 9353K/9712K, paused 3ms+2ms, total 15ms F/GeckoLocalBrowserDB(26281): Reflection error fetching favicon: bookmarkdefaults_title_aboutfirefox F/GeckoLocalBrowserDB(26281): java.lang.NoSuchFieldException: bookmarkdefaults_favicon_aboutfirefox F/GeckoLocalBrowserDB(26281): at java.lang.Class.getField(Class.java:724) F/GeckoLocalBrowserDB(26281): at org.mozilla.gecko.db.LocalBrowserDB.getFaviconId(LocalBrowserDB.java:416) F/GeckoLocalBrowserDB(26281): at org.mozilla.gecko.db.LocalBrowserDB.addDefaultBookmarks(LocalBrowserDB.java:197) F/GeckoLocalBrowserDB(26281): at org.mozilla.gecko.GeckoProfile$1.run(GeckoProfile.java:742) F/GeckoLocalBrowserDB(26281): at android.os.Handler.handleCallback(Handler.java:733) F/GeckoLocalBrowserDB(26281): at android.os.Handler.dispatchMessage(Handler.java:95) F/GeckoLocalBrowserDB(26281): at android.os.Looper.loop(Looper.java:136) F/GeckoLocalBrowserDB(26281): at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:43) E/GeckoLocalBrowserDB(26281): Failed to find favicon resource ID for bookmarkdefaults_title_aboutfirefox D/dalvikvm(26281): GC_CONCURRENT freed 238K, 3% free 10630K/10904K, paused 2ms+5ms, total 23ms F/GeckoLocalBrowserDB(26281): Reflection error fetching favicon: bookmarkdefaults_title_aboutfirefox F/GeckoLocalBrowserDB(26281): java.lang.NoSuchFieldException: bookmarkdefaults_favicon_aboutfirefox F/GeckoLocalBrowserDB(26281): at java.lang.Class.getField(Class.java:724) F/GeckoLocalBrowserDB(26281): at org.mozilla.gecko.db.LocalBrowserDB.getFaviconId(LocalBrowserDB.java:416) F/GeckoLocalBrowserDB(26281): at org.mozilla.gecko.db.LocalBrowserDB.addDefaultBookmarks(LocalBrowserDB.java:199) F/GeckoLocalBrowserDB(26281): at org.mozilla.gecko.GeckoProfile$1.run(GeckoProfile.java:742) F/GeckoLocalBrowserDB(26281): at android.os.Handler.handleCallback(Handler.java:733) F/GeckoLocalBrowserDB(26281): at android.os.Handler.dispatchMessage(Handler.java:95) F/GeckoLocalBrowserDB(26281): at android.os.Looper.loop(Looper.java:136) F/GeckoLocalBrowserDB(26281): at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:43) E/GeckoLocalBrowserDB(26281): Failed to find favicon resource ID for bookmarkdefaults_title_aboutfirefox D/dalvikvm(26310): Trying to load lib /data/app-lib/com.google.android.gms-1/libgmscore.so 0x4232b928 D/dalvikvm(26310): Shared lib '/data/app-lib/com.google.android.gms-1/libgmscore.so' already loaded in same CL 0x4232b928 D/

(In reply to Mark Capella [:capella] from comment #17) > I'm seeing bits like this in logcat after a fresh install ... it's a > |Log.wtf() vs. a Log.i(), so is this expected (warning / safe-to-ignore) ? > No, it's not expected, because that field should exist: > F/GeckoLocalBrowserDB(26281): Reflection error fetching favicon: > bookmarkdefaults_title_aboutfirefox > F/GeckoLocalBrowserDB(26281): java.lang.NoSuchFieldException: > bookmarkdefaults_favicon_aboutfirefox mobile/android/base/strings.xml.in 392: <string name="bookmarkdefaults_title_aboutfirefox">@bookmarks_aboutBrowser@</string> 393: <string name="bookmarkdefaults_url_aboutfirefox">about:firefox</string> 394: <string name="bookmarkdefaults_favicon_aboutfirefox">chrome/chrome/content/branding/favicon64.png</string> It's present in R$string.class in my build output. What's different for you?

I noticed it in my logcat while tracking down Bug 1081768 earlier ... After a complete re=install of m-c repo, wipe of ccache, no local patches, and a complete uninstall on my test device, N7 stock / rooted I'm still seeing this on first install.