848254 - Add support of pre-pinned bookmark on about:home for distributions

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Yuan Xulei [:yxl] (Leave from May 18)]

Spinning this off of bug 834681.

Features:
* Support for pre-pinned bookmarks on about:home
* Support for pre-thumbnails fo the pre-pinned bookmarks.

Comment 1

[:Margaret Leibovic]

Attachment: support for pre-pinned bookmarks

This patch adds support for pre-pinned bookmarks, but doesn't do anything with thumbnails yet.

I changed guidToID to return an Integer so that it would work with my new createBookmark signature, but this seems like the right thing to do anyway, since IDs are all integers in the db (looking at the CREATE TABLE statements in here).

I decided to implement this from the distribution side with a "pin": <integer> property in bookmarks.json, where the integer is the position of the pinned site. I'm open to feedback about any better API for this.

To test this, I just used this bookmarks.json [1]:

[
  {
    "title": "Mozilla",
    "url": "http://mozilla.org/",
    "pin": 2
  }
]

[1] https://wiki.mozilla.org/Mobile/Distribution_Files#Bookmarks

Comment 2

[:Margaret Leibovic]

(Also, this patch saves us a little bit of work by pulling the guidToID calls out of the for loops.)

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 724613 [details] [diff] [review]
support for pre-pinned bookmarks

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

How might we test this?

::: mobile/android/base/db/BrowserProvider.java.in
@@ +1013,5 @@
>              }
>  
>              Locale locale = Locale.getDefault();
>              int pos = 0;
> +            int mobileFolderId = guidToID(db, Bookmarks.MOBILE_FOLDER_GUID);

See later comments. (I review in reverse order!)

@@ +1026,5 @@
>  
> +                    if (bookmark.has("pin")) {
> +                        // Create a fake bookmark in the hidden pinned folder to pin
> +                        // bookmark to about:home top sites.
> +                        int pinPosition = bookmark.getInt("pin");

This like can throw a JSONException if the value cannot be coerced to an int. You probably want to muffle it and simply not pin, rather than blow up.

@@ +1063,5 @@
>              Class<?> stringsClass = R.string.class;
>              Field[] fields = stringsClass.getFields();
>              Pattern p = Pattern.compile("^bookmarkdefaults_title_");
>  
> +            int mobileFolderId = guidToID(db, Bookmarks.MOBILE_FOLDER_GUID);

This will throw a NPE when attempting to unbox a null Integer. Assign to Integer and check for null, or let's extract a method -- getMobileFolderID -- and do this in one place, safely/noisily/with a comment declaring our assumptions.

@@ +1307,5 @@
>              // in the new table given that we've always created it on
>              // database creation time.
>              final int nInvalidSpecialEntries = invalidSpecialEntries.size();
>              if (nInvalidSpecialEntries > 0) {
> +                int mobileFolderId = guidToID(db, Bookmarks.MOBILE_FOLDER_GUID);

Same comment: this will throw a NPE when guidToID returns null (admittedly an unlikely case, but better safe than sorry!).

You're also forcing a coercion back to Integer in ContentValues#put, below. Keep this as Integer, and…

@@ +1312,5 @@
>  
>                  debug("Found " + nInvalidSpecialEntries + " invalid special folder entries");
>                  for (int i = 0; i < nInvalidSpecialEntries; i++) {
>                      ContentValues values = invalidSpecialEntries.get(i);
>                      values.put(Bookmarks.PARENT, mobileFolderId);

… this should change to

  if (mobileFolderId != null) {
      values.put(Bookmarks.PARENT, mobileFolderId);
  }

because ContentValues#putNull is intended for nulls, not #put.

If you really want to ensure that you have a valid parent ID, check and throw a descriptive exception earlier, or see my suggestion about lifting this into a little method.

Comment 4

[:Margaret Leibovic]

Attachment: patch v2

How about this? I decided to turn guidToID into getMobileFolderId, since that method is only used to get the mobile folder id. I added null checks that log and error and bail if the mobile folder id is null. I think we have serious problems if the mobile folder id is null, but I suppose it's better to bail and log an error, rather than crash.

I also added a try/catch for getting the pin position, although I think failing to create a bookmark wouldn't be the end of the world, since this should be tested by whoever is making a distribution.

To test this code, you can put a file at /objdir/dist/bin/distribution/bookmarks.json with the JSON array I included in a comment up above, and when you package the app that will end up in the right place. Also see the wiki page for more details about what can go into these distribution bookmarks.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 724700 [details] [diff] [review]
patch v2

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

::: mobile/android/base/db/BrowserProvider.java.in
@@ +1033,5 @@
> +                        try {
> +                            // Create a fake bookmark in the hidden pinned folder to pin
> +                            // bookmark to about:home top sites.
> +                            int pinPosition = bookmark.getInt("pin");
> +                            createBookmark(db, title, url, pinPosition, Bookmarks.FIXED_PINNED_LIST_ID);

So what happens if more than one pinned item has the same position?

@@ +1791,5 @@
>          try {
>              c = db.query(TABLE_BOOKMARKS,
>                           new String[] { Bookmarks._ID },
>                           Bookmarks.GUID + " = ?",
> +                         new String[] { Bookmarks.MOBILE_FOLDER_GUID },

Let's lift out these two String[] as private static final, immediately before the method definition. I don't believe that they'll be optimized out for you, so this'll save some allocations.

Comment 6

[Wesley Johnston (:wesj)]

(In reply to Richard Newman [:rnewman] from comment #5)
> So what happens if more than one pinned item has the same position?

We will ignore one. LocalBrowserDB has code to avoid this (trying to pin a site will overwrite whatever is already stored at that position).

Comment 7

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to :Margaret Leibovic from comment #1)
> Created attachment 724613 [details] [diff] [review]
> support for pre-pinned bookmarks
> 
> This patch adds support for pre-pinned bookmarks, but doesn't do anything
> with thumbnails yet.
> 
> I changed guidToID to return an Integer so that it would work with my new
> createBookmark signature, but this seems like the right thing to do anyway,
> since IDs are all integers in the db (looking at the CREATE TABLE statements
> in here).
> 
> I decided to implement this from the distribution side with a "pin":
> <integer> property in bookmarks.json, where the integer is the position of
> the pinned site. I'm open to feedback about any better API for this.
> 
> To test this, I just used this bookmarks.json [1]:
> 
> [
>   {
>     "title": "Mozilla",
>     "url": "http://mozilla.org/",
>     "pin": 2
>   }
> ]
> 
> [1] https://wiki.mozilla.org/Mobile/Distribution_Files#Bookmarks

To be clear and succinct, I suggest separate the pinned bookmarks from the default ones and make the bookmarks.json look like this:

{
  mobile: // the default bookmark list
  [
    {
      "title": "Queso",
      "url": "http://queso.com/",
      "icon": "..."
    }
  ],
  pinned: // the pinned bookmark list
  [
    {
      "title": "Queso",
      "url": "http://queso.com/",
      "thumbnail": "..."
    },
    {
      "title": "Mozilla",
      "url": "http://mozilla.org/",
      "thumbnail": "..."
    }
  ] 
}

It'll be clearer to maintain two separated lists than a mixed list. As images required by the two types of bookmark are different. The default bookmark needs icon, while the pinned bookmark need thumbnail. 

Also with this format of JSON, we don't need to take care of the position of the pinned bookmarks.

Comment 8

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

(In reply to Yuan Xulei [:yxl] from comment #7)

> It'll be clearer to maintain two separated lists than a mixed list. As
> images required by the two types of bookmark are different. The default
> bookmark needs icon, while the pinned bookmark need thumbnail. 

I don't think we want two separate lists. It's too verbose and duplicates too much.

We are not going to support setting the thumbnail from JSON. The data is too large and the image size would be fixed, while the actual display changes sizes based on the device.

> Also with this format of JSON, we don't need to take care of the position of
> the pinned bookmarks.

I would not mind just using "pinned: true" in the JSON. Order could be set based on the order of the bookmark.

Comment 9

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

See bug 837392 for a way to get "nicer" thumbnails, without providing the full image.

Comment 10

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Mark Finkle (:mfinkle) from comment #8)
> (In reply to Yuan Xulei [:yxl] from comment #7)
> 
> > It'll be clearer to maintain two separated lists than a mixed list. As
> > images required by the two types of bookmark are different. The default
> > bookmark needs icon, while the pinned bookmark need thumbnail. 
> 
> I don't think we want two separate lists. It's too verbose and duplicates
> too much.
> 
> We are not going to support setting the thumbnail from JSON. The data is too
> large and the image size would be fixed, while the actual display changes
> sizes based on the device.
> 
> > Also with this format of JSON, we don't need to take care of the position of
> > the pinned bookmarks.
> 
> I would not mind just using "pinned: true" in the JSON. Order could be set
> based on the order of the bookmark.

(In reply to Mark Finkle (:mfinkle) from comment #9)
> See bug 837392 for a way to get "nicer" thumbnails, without providing the
> full image.

(In reply to Mark Finkle (:mfinkle) from comment #8)
> (In reply to Yuan Xulei [:yxl] from comment #7)
> 
> > It'll be clearer to maintain two separated lists than a mixed list. As
> > images required by the two types of bookmark are different. The default
> > bookmark needs icon, while the pinned bookmark need thumbnail. 
> 
> I don't think we want two separate lists. It's too verbose and duplicates
> too much.
> 
> We are not going to support setting the thumbnail from JSON. The data is too
> large and the image size would be fixed, while the actual display changes
> sizes based on the device.

The thumbnails provide better user experience and images can be store as files outside the JSON file. 

We can provide a relatively large image suitable to pad and scale it down to support phones with small screen.

The size of a 160*128 thumbnail image could be limited within 10KB and such kind of image is big enough to show in the 10-inch pad.

Two sample images are:

 https://raw.github.com/yxl/mozilla-central/fennec_20_cn/mobile/android/base/cmhomepage-assets/images/amazon.png

 https://raw.github.com/yxl/mozilla-central/fennec_20_cn/mobile/android/base/cmhomepage-assets/images/baidu.png

As the total number of pinned bookmarks is less than 10, the total size of the thumbnail images could be less than 100KB. 

So I think it is acceptable to embed the thumbnails.

> 
> > Also with this format of JSON, we don't need to take care of the position of
> > the pinned bookmarks.
> 
> I would not mind just using "pinned: true" in the JSON. Order could be set
> based on the order of the bookmark.

Comment 11

[Richard Newman [:rnewman]]

(In reply to Yuan Xulei [:yxl] from comment #10)

> As the total number of pinned bookmarks is less than 10, the total size of
> the thumbnail images could be less than 100KB. 

Don't disregard the time and space cost of opening 1-10 files on disk, particularly at first run. I'd really hate to be the person getting those talos emails.

(And if we're loading into ARGB_8888 Bitmaps, those ten thumbnails would be 800KB of heap! I sure hope we aren't doing that for our thumbnails…)

Comment 12

[:Margaret Leibovic]

(In reply to Yuan Xulei [:yxl] from comment #10)

> The size of a 160*128 thumbnail image could be limited within 10KB and such
> kind of image is big enough to show in the 10-inch pad.
> 
> Two sample images are:
> 
>  https://raw.github.com/yxl/mozilla-central/fennec_20_cn/mobile/android/base/
> cmhomepage-assets/images/amazon.png
> 
>  https://raw.github.com/yxl/mozilla-central/fennec_20_cn/mobile/android/base/
> cmhomepage-assets/images/baidu.png

These are favicon images, and we already support including these as data URIs in the bookmark data. Thumbnails would have to be a lot bigger, especially on high-resolution devices.

I agree with rnewman that reading these files on first run would hurt. Since thumbnails don't block support for pre-pinned bookmarks, we can split off this discussion into a separate bug. I think that bug 837392 would be a good compromise solution, especially because we're not considering including thumbnails for our built-in default bookmarks, and those show up in everyone's first run experience.

Comment 13

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #8)

> > Also with this format of JSON, we don't need to take care of the position of
> > the pinned bookmarks.
> 
> I would not mind just using "pinned: true" in the JSON. Order could be set
> based on the order of the bookmark.

I like this idea. This hides implementation detail from the distribution, and it seems silly to want to be able to pin an arbitrary position anyway.

Comment 14

[:Margaret Leibovic]

Attachment: patch v3

Updated to address review comment and mfinkle's comment about the JSON api. 

Sanity-check extra review request :)

Comment 15

[:Margaret Leibovic]

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

Comment 16

[(PTO June 19-July 7) Ryan VanderMeulen]

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