728224 - Create default bookmarks on local DB creation

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Lucas Rocha (:lucasr)]

Madhava, do we want to automatically create default bookmarks on first run?

Comment 1

[Johnathan Nightingale [:johnath]]

Madhava - if we're pre-populating some bookmarks, we'll need strings right quick!

Comment 2

[JP Rosevear [:jpr]]

Is this decided yet?

Comment 3

[Madhava Enros [:madhava]]

Can we use the same default set we use for XUL fennec?

Comment 4

[Lucas Rocha (:lucasr)]

(In reply to Madhava Enros [:madhava] from comment #3)
> Can we use the same default set we use for XUL fennec?

Yes. Implementation-wise, we'd most likely have to store the default bookmarks as an array value in our Android app. I guess we need to account for translations as well?

Comment 5

[Madhava Enros [:madhava]]

The default set in XUL is:

Firefox Start (about:home)
Firefox: Support (http://support.mozilla.com/mobile/)
Firefox: Customize with add-ons (https://addons.mozilla.org/mobile/) *** should this be /android/ instead?
Firefox: About your browser (about:firefox)

Which seems reasonable, with one clarification above about AMO

Comment 6

[Kevin Brosnan [Ex-Mozilla]]

(In reply to Madhava Enros [:madhava] from comment #5)
> The default set in XUL is:
> Firefox: Customize with add-ons (https://addons.mozilla.org/mobile/) ***
> should this be /android/ instead?

yes it should be /android/

Comment 7

[Madhava Enros [:madhava]]

BTW - one additional question -- do we hard-code the en-US, etc., per locale into the urls or do we just let the website redirect?

Comment 8

[Wesley Johnston (:wesj)]

madhava - we just let the websites redirect

I looked at digging out the current bookmarks.json and using it from Java, but doing so means we need to write some sort of JAR reader for Java, as well as knowing the currently locale there. I think we can do both of those, but it seems easier to put these in strings.xml. Alternatively, we could wait until Gecko is loaded and write an API for adding/removing bookmarks or history entries from javascript? That sounds useful for addons, but also like a lot of work?

My plan right now is to use some magic so that we include bookmarks.inc in strings.xml.in when we compile. That means we can share the bookmark titles we have already set up for XUL Fennec with them (no new strings). The actual URL's and favicons are currently kept in bookmarks.json.in. Trying to dig them out of there seems difficult and silly, so I think I will just hardcode them in android_strings.dtd (which copies them into strings.xml during the build).

When we build the Android database, I am looking at using reflection to dig through R.strings for strings that start with "bookmark_title" right now, and using those? That way, future changes will just involve adding some new strings, but not modifying BrowserProvider. That may be overkill, but it doesn't look to hard. That sound ok mfinkle, or you just want everything hardcoded?

Comment 9

[Wesley Johnston (:wesj)]

Attachment: WIP

Whoops. I lied madhava. These url's are localized.

This is a working WIP. Uses the strings from bookmarks.inc. Duplicates the url's from our old implementation (we can remove bookmarks.json I think). Icons are a included as resources, which feels hacky to me, but I can't come up with anything smarter.

So essentially we scan through R.strings looking for anything that looks like "R.string.bookmark_title_*". If we find that, we also look for a string named "R.string.bookmark_url_*". Then we add that to both bookmarks and history (with 1 fake visit so that we'll show up in the initial TopSites query).

I then also load the favicon from R.drawable.bookmark_favicon_* (if that doesn't exist we just throw and bail out with the bookmark added, but no favicon for it). Note that the first time you visit SUMO, they'll override our high res sumo favicon with their 16x16 firefox icon, yay!

This should happen the first time we create this database, so will hit people upgrading from XUL to Android (need to see if the ProfileMigrator will overwrite these entries?)

Still cleaning up and trying to think of a better way to handle favicons, so only asking for feedback.

Comment 10

[Wesley Johnston (:wesj)]

Attachment: WIP2

I like this a bit better. I added a method Favicons.getFallback(Context, String url) that we can use for the awesomescreen, and for favicons in general. I'm a bit worried about the performance impact of that, but I'm caching the two icons I use, and think it should be negligible.

I worked on providing special fallbacks for things like about:neterror, but turns out in that case we get the url of the page, and a jar protocol favicon. I don't want to let anything but about pages get access to special favicons, so most of my ideas were bust.

In fact, I like it enough that I'm marking review, but happy to look for other alternatives.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 605639 [details] [diff] [review]
WIP2

>diff --git a/mobile/android/base/AwesomeBarTabs.java b/mobile/android/base/AwesomeBarTabs.java

>             if (b == null) {
>-                favicon.setImageDrawable(null);
>+                int urlIndex = cursor.getColumnIndexOrThrow(URLColumns.URL);
>+                favicon.setImageBitmap(Favicons.getFallback(mContext, cursor.getString(urlIndex)));

No thank you. We want to show "nothing" for favicons in the awesomebar list

>diff --git a/mobile/android/base/Favicons.java b/mobile/android/base/Favicons.java

>+    public static Bitmap getFallback(Context aContext, String aPageUrl) {
>+        if (aPageUrl.startsWith("about:")) {
>+            return getAboutFavicon(aContext);
>+        } else {
>+            return getEmptyFavicon(aContext);
>+        }
>+    }

Return null, not Empty

>+    static private Bitmap aboutFavicon = null;
>+    static private Bitmap getAboutFavicon(Context aContext) {
>+        if (aboutFavicon == null)
>+            aboutFavicon = BitmapFactory.decodeResource(aContext.getResources(), R.drawable.about_page_favicon);
>+        return aboutFavicon;
>+    }

Also, this is bug 709250. You are kinda wallpapering over it.

>+
>+    static private Bitmap emptyFavicon = null;
>+    static private Bitmap getEmptyFavicon(Context aContext) {
>+        if (emptyFavicon == null)
>+            emptyFavicon = BitmapFactory.decodeResource(aContext.getResources(), R.drawable.favicon);
>+        return emptyFavicon;
>+    }

I don't think we want this at all

>             } catch (Exception e) {
>                 Log.d(LOGTAG, "Error downloading favicon: " + e);
>+                Bitmap bitmap = getFallback(mContext, mPageUrl);
>+                return new BitmapDrawable(bitmap);

If we decide to keep your "about:" fallback I guess we would keep this. But remove if we remove the "about:" fallback

>             } catch (MalformedURLException e) {
>-                Log.d(LOGTAG, "The provided favicon URL is not valid: " + e);
>-                return null;
>+                Log.e(LOGTAG, "The provided favicon URL is not valid", e);
>+                Bitmap bitmap = getFallback(mContext, mPageUrl);
>+                return new BitmapDrawable(bitmap);

Same

>diff --git a/mobile/android/base/Makefile.in b/mobile/android/base/Makefile.in

>+  res/drawable/bookmark_favicon_support.png \
>+  res/drawable/bookmark_favicon_addons.png \
>+  res/drawable/about_page_favicon.png \

I think we want to remove the about_page_favicon if we don't want the "about:" fallback too.

>diff --git a/mobile/android/base/db/BrowserProvider.java.in b/mobile/android/base/db/BrowserProvider.java.in

>+            createDefaultBookmarks(db, "^bookmark_title_");

I kinda like "bookmarkdefaults_" as the prefix for the title, urls and favicons. It's more explicit.

>+        private void createDefaultBookmarks(SQLiteDatabase db, String pattern) {

>+            ContentValues historyValues = new ContentValues();
>+            historyValues.put(History.VISITS, 1);
>+            historyValues.put(History.DATE_LAST_VISITED, now);

add: // XXX - This should be removed when bug 721731 is fixed

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd

>+<!-- Default bookmarks -->
>+<!ENTITY bookmark_url_aboutfirefox "about:firefox">
>+<!ENTITY bookmark_url_addons "https://addons.mozilla.org/@AB_CD@/mobile/">
>+<!ENTITY bookmark_url_support "http://support.mozilla.org/@AB_CD@/mobile">
>+<!ENTITY bookmark_url_abouthome "about:home">

I am a little sad that the URL and Titles are not "together". I'll get over it I guess.

>\ No newline at end of file

fix

r+ with the nits fixed. you should also make a simple test to check that the bookmarks are created.

Asking Pike for feedback on the Makefile parts and to give him (and l10n) a heads up on some string slippage. Even though the strings are URLs.

Comment 12

[Axel Hecht [:Pike]]

Comment on attachment 605639 [details] [diff] [review]
WIP2

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

r-, the build logic isn't going to work, and we shouldn't put the urls into the localizable files. That's the reason why we preprocess bookmarks.json and bookmarks.html on desktop in the first place.

Tips on how to do that below.

::: mobile/android/base/locales/Makefile.in
@@ +68,5 @@
>  	$(PYTHON) $(topsrcdir)/config/Preprocessor.py $(DEFINES) \
>  	  -DBRANDPATH="$(BRANDPATH)" \
>  	  -DSTRINGSPATH="$(STRINGSPATH)" \
>  	  -DSYNCSTRINGSPATH="$(SYNCSTRINGSPATH)" \
> +	  -DBOOKMARKSPATH="../../locales/$(AB_CD)/profile/bookmarks.inc" \

This is not going to work, you'll need MERGEFILE, but that depends on relativesrcdir, which is different for bookmarks.inc. As does LOCALE_SRCDIR inside there. I guess the easiest way is to just copy and tweak the code for that from rules.mk, with a comment that you're doing that:

EXPAND_LOCALE_SRCDIR = $(if $(filter en-US,$(AB_CD)),$(topsrcdir)/$(1)/en-US,$(call core_realpath,$(L10NBASEDIR))/$(AB_CD)/$(subst /locales,,$(1)))

ifdef LOCALE_MERGEDIR
MERGE_FILE = $(firstword \
  $(wildcard $(LOCALE_MERGEDIR)/$(subst /locales,,$(relativesrcdir))/$(1)) \
  $(wildcard $(LOCALE_SRCDIR)/$(1)) \
  $(srcdir)/en-US/$(1) )
else
MERGE_FILE = $(LOCALE_SRCDIR)/$(1)

would then become

MOBILE_LOCALE_SRCDIR = $(if $(filter en-US,$(AB_CD)),$(topsrcdir)/mobile/locales/en-US,$(call core_realpath,$(L10NBASEDIR))/$(AB_CD)/mobile

ifdef LOCALE_MERGEDIR
BOOKMARKSPATH = $(firstword \
  $(wildcard $(LOCALE_MERGEDIR)/mobile/profile/bookmarks.inc) \
  $(wildcard $(MOBILE_LOCALE_SRCDIR)/profile/bookmarks.inc) \
  $(topsrcdir)/mobile/locales/en-US/profile/bookmarks.inc )
else
MERGE_FILE = $(MOBILE_LOCALE_SRCDIR)/profile/bookmarks.inc

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +115,5 @@
> +<!-- Default bookmarks -->
> +<!ENTITY bookmark_url_aboutfirefox "about:firefox">
> +<!ENTITY bookmark_url_addons "https://addons.mozilla.org/@AB_CD@/mobile/">
> +<!ENTITY bookmark_url_support "http://support.mozilla.org/@AB_CD@/mobile">
> +<!ENTITY bookmark_url_abouthome "about:home">
\ No newline at end of file

We shouldn't add those to l10n, that's the base reason for preprocessing bookmarks.json or bookmarks.html for desktop.

::: mobile/android/base/strings.xml.in
@@ +127,5 @@
> +  <string name="bookmark_title_support">@bookmarks_support@</string>
> +  <string name="bookmark_url_support">&bookmark_url_support;</string>
> +
> +  <string name="bookmark_title_abouthome">@bookmarks_aboutHome@</string>
> +  <string name="bookmark_url_abouthome">&bookmark_url_abouthome;</string>

Just hardcode the urls here, and add a comment why the urls are here and yet hardcoded.

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Revised patch. I wrote a nested jar decoder and put that in bug 709250, so some of this code will go away there.

This probably breaks a bookmarks pane test again as well. Need to test and will post a second patch for that.

Comment 14

[Wesley Johnston (:wesj)]

I should point out mark, this causes the "Firefox: Customize with addons" and "Firefox Support" to appear on about:home on firstrun, with no thumbnails. That will be fixed when bookmarks in awesomescreen lands and we can remove the code that adds fake history visits for these.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 607369 [details] [diff] [review]
Patch v2


>diff --git a/mobile/android/base/AwesomeBarTabs.java b/mobile/android/base/AwesomeBarTabs.java
>             if (b == null) {
>-                favicon.setImageDrawable(null);
>+                int urlIndex = cursor.getColumnIndexOrThrow(URLColumns.URL);
>+                Bitmap bitmap = Favicons.getFallback(mContext, cursor.getString(urlIndex));
>+                favicon.setImageBitmap(bitmap);

Call me crazy, but I'd prefer if this was reverted in this patch instead of the jar:jar: patch

>diff --git a/mobile/android/base/Favicons.java b/mobile/android/base/Favicons.java

>+    public static Bitmap getFallback(Context aContext, String aPageUrl) {
>+        if (aPageUrl.startsWith("about:")) {
>+            return getAboutFavicon(aContext);
>+        }
>+        return null;
>+    }
>+
>+    // xxx - remove when bug 709250 is fixed
>+    static private Bitmap aboutFavicon = null;
>+    static private Bitmap getAboutFavicon(Context aContext) {
>+        if (aboutFavicon == null)
>+            aboutFavicon = BitmapFactory.decodeResource(aContext.getResources(), R.drawable.about_page_favicon);
>+        return aboutFavicon;
>+    }
>+

Same.

>             } catch (Exception e) {
>                 Log.d(LOGTAG, "Error downloading favicon: " + e);
>+                Bitmap bitmap = getFallback(mContext, mPageUrl);
>+                return new BitmapDrawable(bitmap);

Same.

>             } catch (MalformedURLException e) {
>-                Log.d(LOGTAG, "The provided favicon URL is not valid: " + e);
>-                return null;
>+                Log.e(LOGTAG, "The provided favicon URL is not valid", e);
>+                Bitmap bitmap = getFallback(mContext, mPageUrl);
>+                return new BitmapDrawable(bitmap);

Same.

>diff --git a/mobile/android/base/db/BrowserProvider.java.in b/mobile/android/base/db/BrowserProvider.java.in

>+        private void createDefaultBookmarks(SQLiteDatabase db, String pattern) {

>+            ContentValues historyValues = new ContentValues();
>+            // XXX - This should be removed when bug 721731 is fixed
>+            historyValues.put(History.VISITS, 1);
>+            historyValues.put(History.DATE_LAST_VISITED, now);

Put the comment on top

r+ with the nits fixed and Axel's blessing on the Makefile stuff

Comment 16

[Axel Hecht [:Pike]]

Comment on attachment 607369 [details] [diff] [review]
Patch v2

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

r=me, with a a bit more comments?

::: mobile/android/base/locales/Makefile.in
@@ +71,5 @@
> +  $(wildcard $(MOBILE_LOCALE_SRCDIR)/profile/bookmarks.inc) \
> +  $(topsrcdir)/mobile/locales/en-US/profile/bookmarks.inc ))
> +else
> +BOOKMARKSPATH = $(MOBILE_LOCALE_SRCDIR)/profile/bookmarks.inc
> +endif

Add a comment on top of this block that it's a copy-and-tweak of MERGE_FILE in config/config.mk?

::: mobile/android/base/strings.xml.in
@@ +124,5 @@
>    <string name="abouthome_no_top_sites">&abouthome_no_top_sites;</string>
>    <string name="abouthome_about_sync">&abouthome_about_sync;</string>
>    <string name="abouthome_sync_bold_name">&abouthome_sync_bold_name;</string>
> +
> +  <!-- Default bookmarks -->

An additional comment here would be great, something like:

Use shared bookmarks titles from mobile's profile/bookmarks.inc, don't expose the URLs to l10n.

Comment 17

[Wesley Johnston (:wesj)]

Attachment: Patch

Carrying forward +

Comment 18

[Wesley Johnston (:wesj)]

Attachment: Tests

I split this test in two. One for the bookmarks list. One for the menu button.

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 608123 [details] [diff] [review]
Tests


>-public class testBookmark extends BaseTest {
>+public class testBookmark extends BaseTest  {

extra space

>+        BOOKMARK_URL = getAbsoluteUrl(BOOKMARK_URL);

it seems wrong to be doing it like this (all caps static getting updated) but I can live with it

>+        mClassLoader = getActivity().getApplicationContext().getClassLoader();
>+        try {
>+            Class browserDB = mClassLoader.loadClass("org.mozilla.gecko.db.BrowserDB");
>+            mAddBookmark = browserDB.getMethod("addBookmark", ContentResolver.class, String.class, String.class);
>+            mRemoveBookmark = browserDB.getMethod("removeBookmarksWithURL", ContentResolver.class, String.class);
>+            mIsBookmarked = browserDB.getMethod("isBookmark", ContentResolver.class, String.class);
>+        } catch (java.lang.ClassNotFoundException ex) {
>+            mAsserter.is(true, false, "Unable to find class");
>+        } catch (java.lang.NoSuchMethodException ex) {
>+            mAsserter.is(true, false, "Unable to find method");
>+        }

oh the crazy stuff we need to do for tests to work...

Comment 20

[Lucas Rocha (:lucasr)]

Comment on attachment 608122 [details] [diff] [review]
Patch

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

I'd like to see those issues addressed and questions answered before landing.

::: mobile/android/base/Favicons.java
@@ +335,5 @@
>                  // If favicon is empty, fallback to default favicon URI
>                  if (mFaviconUrl == null || mFaviconUrl.length() == 0) {
>                      // Handle the case of malformed URL
>                      URL pageUrl = null;
> +                    pageUrl = new URL(mPageUrl);

How is this related at all with adding default bookmarks? Also, why don't you need the try/catch block anymore?

::: mobile/android/base/Makefile.in
@@ +89,5 @@
>    GeckoEventListener.java \
>    GeckoEventResponder.java \
>    GeckoHalDefines.java \
>    GeckoInputConnection.java \
> +  GeckoMessageReceiver.java \

Unrelated change. No big deal though, just pointing out.

@@ +97,5 @@
>    GeckoThread.java \
>    GlobalHistory.java \
>    LinkPreference.java \
>    LinkTextView.java \
> +  NSSBridge.java \

Unrelated change. No big deal though, just pointing out.

@@ +357,5 @@
>    res/drawable/site_security_identified.png \
>    res/drawable/site_security_verified.png \
>    res/drawable/urlbar_stop.png \
> +  res/drawable/bookmarkdefaults_favicon_support.png \
> +  res/drawable/bookmarkdefaults_favicon_addons.png \

Where are those images used? I couldn't find any reference in the patch.

::: mobile/android/base/db/BrowserProvider.java.in
@@ +343,5 @@
>                  R.string.bookmarks_folder_places, 0);
>  
>              createOrUpdateAllSpecialFolders(db);
>  
> +            createDefaultBookmarks(db, "^bookmarkdefaults_title_");

Is createDefaultBookmarks called anywhere else? I guess you need this argument with the pattern them? Just hardcode it in the method, no?

@@ +348,5 @@
> +        }
> +
> +        private void createDefaultBookmarks(SQLiteDatabase db, String pattern) {
> +            Class stringsClass = R.string.class;
> +            Class drawablesClass = R.drawable.class;

nit: add an empty line here.

@@ +353,5 @@
> +            Field[] fields = stringsClass.getFields();
> +            Pattern p = Pattern.compile(pattern);
> +
> +            ContentValues bookmarksValues = new ContentValues();
> +            bookmarksValues.put(Bookmarks.PARENT, getFolderIdFromGuid(db, Bookmarks.MOBILE_FOLDER_GUID));

We already have guidToID().

@@ +358,5 @@
> +            long now = System.currentTimeMillis();
> +            bookmarksValues.put(Bookmarks.DATE_CREATED, now);
> +            bookmarksValues.put(Bookmarks.DATE_MODIFIED, now);
> +
> +            // XXX - This should be removed when bug 721731 is fixed

Maybe just remove the code and wait for bug 721731 to land? It's very close to land anyway and saves you the work of removing this code later.

@@ +367,5 @@
> +            int pos = 1;
> +            for (int i = 0; i < fields.length; i++) {
> +                String name = fields[i].getName();
> +                Matcher m = p.matcher(name);
> +                if (m.find()) {

Do an early return instead? It would avoid one more indentation level here. Something like:

if (!m.find())
    return;

@@ +372,5 @@
> +                    try {
> +                        int titleid = fields[i].getInt(null);
> +                        String title = mContext.getString(titleid);
> +
> +                        Field urlField = stringsClass.getField(name.replace("_title_", "_url_"));

Hmm, a bit hacky, no? Why not having the field names in predefined constants instead?

@@ +386,5 @@
> +                        historyValues.put(History.TITLE, title);
> +                        historyValues.put(History.URL, url);
> +                        id = db.insertOrThrow(TABLE_HISTORY, History.VISITS, historyValues);
> +
> +                        // Look for a drawable with the id R.drawable.bookmarkdefaults_favicon_*

Factor out this code to fetch the favicon into a separate private method.

@@ +387,5 @@
> +                        historyValues.put(History.URL, url);
> +                        id = db.insertOrThrow(TABLE_HISTORY, History.VISITS, historyValues);
> +
> +                        // Look for a drawable with the id R.drawable.bookmarkdefaults_favicon_*
> +                        Field faviconField = drawablesClass.getField(name.replace("_title_", "_favicon_"));

Again, maybe have those field names predefined in constants? It would make this code more readable.

I noticed that you don't define any favicons in string.xml.in. Where are these favicons coming from then? Some sort of fallback mechanism?

@@ +388,5 @@
> +                        id = db.insertOrThrow(TABLE_HISTORY, History.VISITS, historyValues);
> +
> +                        // Look for a drawable with the id R.drawable.bookmarkdefaults_favicon_*
> +                        Field faviconField = drawablesClass.getField(name.replace("_title_", "_favicon_"));
> +                        if (faviconField != null) {

In the separate method, do an early return if faviconField is null.

@@ +399,5 @@
> +                            values.put(Images.FAVICON, stream.toByteArray());
> +                            values.put(Images.URL, url);
> +                            values.put(Images.IS_DELETED, 0);
> +                            id = db.insertOrThrow(TABLE_IMAGES, Images.URL, values);
> +                        }

nit: add empty line here.

@@ +410,5 @@
> +                }
> +            }
> +        }
> +
> +        private long getFolderIdFromGuid(SQLiteDatabase db, String guid) {

There's a guidToID method somewhere in BrowserProvider already. Isn't that enough?

Comment 21

[Lucas Rocha (:lucasr)]

Comment on attachment 608123 [details] [diff] [review]
Tests

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

The test for bookmarks seems a bit hand wave-y... Strictly speaking, just checking the number of initial bookmarks doesn't "prove" that you default bookmark setup is working as expected. Maybe add a note about that in the code or maybe file a follow up? The thing with the test follow ups is that we tend to just forget about them so I suggest to get the tests solid on the first go instead of doing it later. But feel free to ignore me :-)

It seems like you're adding two types of tests on the same patch here. One for adding/deleting bookmarks from the UI and another for checking the default bookmarks. I'd probably add those in different patches. Just a suggestion.

::: mobile/android/base/tests/testBookmark.java.in
@@ +70,5 @@
>          mAsserter.ok(bookmarksList != null, "checking that bookmarks list exists", "bookmarks list exists");
>  
>          // No folders should be visible if no desktop bookmarks exist
> +        mAsserter.is(bookmarksList.getChildCount(), 5,
> +            "bookmarks list has 5 children (the default bookmarks and the hidden header)");

Shouldn't you check which bookmarks are there and compare with the expected list of default bookmarks? How do you guarantee that this test will run from a fresh database? Does our infra guarantee a fresh profile for every test run? I mean, if some other UI test adds a bookmark to the DB, will this new bookmark show here?

Comment 22

[Wesley Johnston (:wesj)]

> How is this related at all with adding default bookmarks? Also, why don't
> you need the try/catch block anymore?

Its not related anymore. Originally, I had some logic here to show a "default" favicon for about pages. I removed it, but figured I would leave this bit of cleanup in. This try catch is inside another try catch. While they provide slightly different logging messages, there's no other reason to have both. I personally don't think the log message is worth the extra code complexity.

> Unrelated change. No big deal though, just pointing out.

Yes. My stupid code editor likes to put tabs in Makefiles. It embarrasses me, so I sneak the fixes by in unrelated patches.

> Where are those images used? I couldn't find any reference in the patch.

They're dynamically looked up. i.e. we search through strings. Find one named R.id.bookmarkdefaults_title_xxx and then search for the R.drawable.bookmarkdefaults_favicon_xxx to use for its favicon. I wrote things this way to minimize changes needed to add or change these defaults.

I'd like to, in addition, first search for a string bookmarkdefaults_favicon_xxx and use that path if we can, but we don't have a chrome-uri parser in Java (and keeping one in sync with registered chrome resources in Gecko is probably non-trivial?) and we can't hardcode the path to fennec.apk. I still want do this, but have it point to a relative path within fennec.apk. Then, at run time, I can append all of the jar-uri pointing to omni.ja. Right now, since I removed their defaults, the two about-urls have no favicon initially. I figured that would be a fine follow up rather than iterating on this again.

> Is createDefaultBookmarks called anywhere else? I guess you need this
> argument with the pattern them? Just hardcode it in the method, no?

createDefaultBookmarks is only called in BrowserProvider.DatabaseHelper.OnCreate, but I thought allowing you to pass in a parameter might be nice if we ever needed to do a migration or something. Maybe you could search for "^bookmarkdefaults_v2_title_". Likely thats just fanciful dreaming.

> We already have guidToID().

Whoops. Thanks!

> Hmm, a bit hacky, no? Why not having the field names in predefined constants
> instead?

Again, I was trying to be tricky here and minimize the work needed to change these bookmarks (i.e, right now you just need to add them to strings.xml.in and add a localized title in mobile/locales/en-US/profile/bookmarks.inc or mobile/android/base/locales/en-US/android_strings.dtd).

Comment 23

[Wesley Johnston (:wesj)]

Attachment: Patch

We'll go for the r+ hat-trick here I guess. This addresses the nits, but not the things I brought up above.

Comment 24

[Wesley Johnston (:wesj)]

Attachment: tests

Tests. Added some tests for the urls. I don't like testing for particular strings (although we do it all over here), so I want to avoid testing the titles.

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 608383 [details] [diff] [review]
Patch

This looks good enough to land. If anything needs more attention, we can file a followup bug.

Comment 26

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fa800f5ff0c
https://hg.mozilla.org/integration/mozilla-inbound/rev/25b7ced6fdd7

Comment 27

[Ed Morley [:emorley]]

Please can you set the target milestone when landing on inbound, along the lines of http://blog.bonardo.net/2012/03/23/how-you-can-help-mozilla-inbound-sheriffs-when-pushing :-)

https://hg.mozilla.org/mozilla-central/rev/25b7ced6fdd7
https://hg.mozilla.org/mozilla-central/rev/5fa800f5ff0c

Comment 28

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch to fix makefile

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/603d27ac23e2

Comment 30

[Paul Feher]

Verified/fixed on:
Nightly Fennec 14.0a1 (2012-04-02)
Device: Samsung Nexus S
OS: Android 2.3.6

Comment 31

[Wesley Johnston (:wesj)]

Comment on attachment 608383 [details] [diff] [review]
Patch

[Approval Request Comment]
Regression caused by (bug #): New feature
User impact if declined: No bookmarks for new users
Testing completed (on m-c, etc.): On mc for a week now
Risk to taking this patch (and alternatives if risky): Touches android only databases. some risk
String changes made by this patch: None!

Comment 32

[Wesley Johnston (:wesj)]

Comment on attachment 608386 [details] [diff] [review]
tests

[Approval Request Comment]
Regression caused by (bug #): New feature
User impact if declined: Fixes and adds Tests for feature
Testing completed (on m-c, etc.): On mc for a week now
Risk to taking this patch (and alternatives if risky): Low risk
String changes made by this patch: None!

Comment 33

[Wesley Johnston (:wesj)]

Comment on attachment 608386 [details] [diff] [review]
tests

I take this back. I don't think we need these on Aurora unless we're planning to ship from there. Un-noming. If someone wants to push something that depends on these, we can talk about it then.