Brad Lassey [:blassey] (use needinfo?) Reporter
	Description • 10 years ago

      No description provided.

	Brad Lassey [:blassey] (use needinfo?) Reporter
	Comment 1 • 10 years ago

http://ericnelson.wordpress.com/2012/06/10/pinning-sites-in-windows-8-add-two-meta-tags-to-make-it-high-quality/

	Brad Lassey [:blassey] (use needinfo?) Reporter
	Comment 2 • 10 years ago

this is a work in progress. I think we need to add some local caching of the images and perhaps of the DB query results.

	Brad Lassey [:blassey] (use needinfo?) Reporter
	Comment 3 • 10 years ago

This is how it looks with the patch, CNN, Twitter and ABC provide images in their meta data. It seems google properties don't (at least to our UA) and neither does facebook or the boston globe. And... that's the extent of my testing.

r? to Ian to see if he likes the way this looks.

	Brad Lassey [:blassey] (use needinfo?) Reporter
	Comment 4 • 10 years ago

Comment on attachment 8436196 [details] [diff] [review]
tile_images.patch

Margaret & Lucas, Finkle asked me to get feedback from you two on this. Essentially we're observing when these meta tags appear in documents and sending messages to Java to record that to a DB. When topsites shows a tile for a url it checks with the DB to see if has an image and color specified by the site itself to use.

I'm fairly certain we should be doing some sort of local caching of these images. I also wouldn't be surprised if this isn't the right database choice.

	Ian Barlow (:ibarlow)
	Comment 5 • 10 years ago

Huh, cool. Is there any kind of standardization on what size icon we get? Or will it be different for every site?

	Brad Lassey [:blassey] (use needinfo?) Reporter
	Comment 6 • 10 years ago

144x144px

	Wesley Johnston (:wesj) Assignee
	Comment 7 • 10 years ago

Is this missing SiteTiles.java?

	Ian Barlow (:ibarlow)
	Comment 8 • 10 years ago

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #6)
> 144x144px

Ok. Well, it is definitely an improvement over our current favicons. Worth landing and living with it for a while to see how it feels. I'm assuming the idea here is to use these not only in place of favicons but also in place of screenshots?

	Brad Lassey [:blassey] (use needinfo?) Reporter
	Comment 9 • 10 years ago

Yup, forgot to hg add SiteTiles.java

Ian, yes this patch used these images over favicons and thumbnails, but preferst suggested site graphics.

	Ian Barlow (:ibarlow)
	Comment 10 • 10 years ago

Comment on attachment 8436197 [details]
2014-06-06 20.13.27.png

forgot to r+

	:Margaret Leibovic
	Comment 11 • 10 years ago

Comment on attachment 8437650 [details] [diff] [review]
tile_images.patch

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

As a general approach this seems fine to me. I like the idea of keeping this information in a separate db, rather than stuffing even more stuff into browser.db. Also, this seems like a pretty easy way to get some richer data for the UI.

::: mobile/android/base/GeckoApp.java
@@ +1555,4 @@
>              "Update:Download",
>              "Update:Install");
>  
> +        SiteTiles.getInstance();

I understand you need to do this to register a gecko event listener, but it would be really nice if we could avoid adding this dependency to GeckoApp.

::: mobile/android/base/db/BrowserDB.java
@@ +418,5 @@
> +            return Color.parseColor(bgColor);
> +        }
> +
> +        return 0;
> +    }

Why are these methods added to BrowserDB? It seems like it would be more straightforward to have consumers use SiteTiles directly.

::: mobile/android/base/db/SiteTiles.java
@@ +1,1 @@
> +package org.mozilla.gecko.db;

Remember to add a license header.

@@ +25,5 @@
> +        return sInstance;
> +    }
> +
> +    private SiteTiles() {
> +        super(GeckoAppShell.getContext(), "SiteTiles", null, 2);

We need to do something more complex here if we want this db to be profile-aware. I would say we don't need to, since this is just a collection of images from the internet, but for privacy reasons we would need a way to clear this.

I was looking for examples of where we currently use SQLiteOpenHelper, and we usually pass around database file paths.

::: mobile/android/chrome/content/browser.js
@@ +6004,5 @@
> +            let tab = BrowserApp.getTabForBrowser(browser);
> +            if (tab)
> +               this.updateMetadata(tab, false);
> +            }
> +            break;

Looks like you're missing a close brace here.

You should make sure these case statements are enclosed in braces, since you're declaring local variables in here. If you don't, they will leak into the large scope. Actually, it looks like the outer switch statement here is currently doing that wrong :/

@@ +6014,5 @@
> +                uri: browser.currentURI.spec,
> +                contentType: type,
> +                content: type == "image" ? browser.currentURI.resolve(target.content) : target.content
> +              };
> +	      sendMessageToJava(msg);

Nit: indentation is off here.

	Lucas Rocha (:lucasr)
	Comment 12 • 10 years ago

Comment on attachment 8437650 [details] [diff] [review]
tile_images.patch

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

I wonder whether this should be integrated with SuggestedSites somehow or not given how similar they look.

::: mobile/android/base/db/SiteTiles.java
@@ +12,5 @@
> +import android.database.sqlite.SQLiteDatabase;
> +import android.database.sqlite.SQLiteOpenHelper;
> +import android.util.Log;
> +
> +public class SiteTiles extends SQLiteOpenHelper implements GeckoEventListener {

I wonder if 'tiles' is a good name for this. Maybe SiteImages or something?

@@ +39,5 @@
> +            String contentType = message.getString("contentType");
> +            String content = message.getString("content");
> +            Log.i(LOGTAG, "got " + uriSpec + " " + message.getString("contentType") + " " + message.getString("content"));
> +	    SQLiteDatabase db = getWritableDatabase();
> +	    Cursor cursor = db.query(TABLE_NAME, new String[] {"image", "color"}, "url='"+uriSpec+"'", null, null, null, null, null);

This is doing blocking I/O in the gecko thread. Likely to cause page load regressions, no? I'd push these operations to the background thread.

@@ +49,5 @@
> +		values.put("color", cursor.getString(1));
> +	    }
> +	    cursor.close();
> +            values.put(contentType, content);
> +	    db.insertWithOnConflict("tiles", null, values, SQLiteDatabase.CONFLICT_REPLACE);

I'd probably just add this table to our existing browser.db (beside favicons, thumbnails, etc) and use a ContentProvider/BrowserContract as the API to fetch/store this data. You'll also be able to reuse our existing DB code.

::: mobile/android/base/db/SuggestedSites.java
@@ +257,4 @@
>          final Site site = getSiteForUrl(url);
>          return (site != null ? site.bgColor : null);
>      }
> +}

Unrelated?

::: mobile/android/base/home/TopSitesPanel.java
@@ +557,5 @@
>              }
>  
> +            // Suggested images have precedence over thumbnails, no need to wait
> +            // for them to be loaded. See: CursorLoaderCallbacks.onLoadFinished()
> +            if (BrowserDB.hasSpecifiedImageUrl(url)) {

This is going to perform a blocking I/O in the main thread. Build a mem-cached list of images and use that for the contains()/getSpecificedImageUrlForUrl()/getSpecificedBackgroundColorforUrl operations.

	Brad Lassey [:blassey] (use needinfo?) Reporter
	Comment 13 • 10 years ago

Margaret & Lucas, your feedback is directly opposing over using browser.db and integrating with SuggestedSites. Discuss.

	:Margaret Leibovic
	Comment 14 • 10 years ago

We should probably think about our long terms plans here. I believe thumbnails and favicons were added to browser.db originally because we did joins with bookmarks/history to use those images, but we've been moving towards just loading them asynchronously. I thought rnemwman had a plan to break these things apart at one point, but maybe I'm just thinking about breaking BrowserProvider into different content providers.

I'm not a SQLite expert, but I wonder what the tradeoffs are between one large databse with a bunch of tables vs. multiple smaller databases. I would guess the latter would help us avoid things like accidental concurrency and database locking problems.

That being said, using the existing browser.db code would help you with things like being profile aware, so that may be an easier path forward for now.

	Richard Newman [:rnewman]
	Comment 15 • 10 years ago

I don't think there's a bug on file for storing images outside of browser.db, but I am generally in favor of doing that.

Having only one DB for this kind of thing has significant advantages:

* Fewer coordination problems (e.g., one DB failing to open). For example, if we clear a site from history, we should drop the tile image.
* Fewer upgrade problems -- if there's anything worse than a schema update, it's updating two DBs in parallel.
* Less effort in doing per-profile storage.
* Only one DB connection to occupy memory and require initialization.
* Ability to do joins across data types (e.g., fetch tile/thumbnail/favicon).

Mostly those are arguments against having multiple SQLite databases, or otherwise storing structured data outside of browser.db.


If these tiles are relatively common, their size becomes an issue. CNN's is 3KB, but ABC's is a whopping 28KB.

Unlike suggested sites, we should be expecting these tiles to be expired from our cache, and eliminated when private data is cleared.

So I would recommend the following course of action:

* Metadata (page URL => tile URL) lives in browser.db. Joins, clearing, profile support becomes easier.

* Image files live elsewhere, ideally not in a SQLite DB -- in the network cache or on disk, it's all the same to me. Maybe we can call this the Site Image Cache or something.

* Eventually, thumbnails (and perhaps even favicons) can suffer the same fate.

The only other course of action I'd see as acceptable is treating tiles exactly like thumbnails, with the same cleanup and limiting.


Avenues I would veto:

* Storing tiles in browser.db without a limiting/cleanup scheme.
* Storing tiles in a separate SQLite database.

	Brad Lassey [:blassey] (use needinfo?) Reporter
	Comment 16 • 10 years ago

I don't think we should be storing the actual images in the DB. Instead, I'd propose storing the URL and a file path to where it's been downloaded to. If that file path empty, it needs to be downloaded. Also, they should be stored in the android cache dir.

	Wesley Johnston (:wesj) Assignee
	Comment 17 • 10 years ago

I've always hoped we'd just store the url's and moved to using Picasso for the display. It has logic built in for caching and expiry.

	:Margaret Leibovic
	Comment 18 • 10 years ago

(In reply to Wesley Johnston (:wesj) from comment #17)
> I've always hoped we'd just store the url's and moved to using Picasso for
> the display. It has logic built in for caching and expiry.

I thought it only had an in-memory cache. Does it cache images between app runs? If so, this could be an opportunity to try that out.

	Richard Newman [:rnewman]
	Comment 19 • 10 years ago

> I thought it only had an in-memory cache. Does it cache images between app
> runs? If so, this could be an opportunity to try that out.

If not, should we make it do that, rather than building our own independent disk cache?

	Wesley Johnston (:wesj) Assignee
	Comment 20 • 10 years ago

There homepage says "Automatic memory and disk caching."

http://square.github.io/picasso/

	Lucas Rocha (:lucasr)
	Comment 21 • 10 years ago

(In reply to :Margaret Leibovic from comment #18)
> (In reply to Wesley Johnston (:wesj) from comment #17)
> > I've always hoped we'd just store the url's and moved to using Picasso for
> > the display. It has logic built in for caching and expiry.
> 
> I thought it only had an in-memory cache. Does it cache images between app
> runs? If so, this could be an opportunity to try that out.

Memory and disk caching are implemented in separate parts of Picasso's architecture. Memory caching is directly exposed via the Cache interface and they provide a default LRU memcache. Disk caching is assumed to be done on your Downloader implementation. The default Downloader (the one we're using) is based on UrlConnection which only supports disk caching on ICS+.
 
So, yes, Picasso gives us memory (all versions) and disk caching (ICS+). Not sure it's worth implementing a fully-fledged image caching mechanism just to extend Picasso's disk caching support to pre-ICS devices.

	Lucas Rocha (:lucasr)
	Comment 22 • 10 years ago

(In reply to Lucas Rocha (:lucasr) from comment #21)
> So, yes, Picasso gives us memory (all versions) and disk caching (ICS+). Not
> sure it's worth implementing a fully-fledged image caching mechanism just to
> extend Picasso's disk caching support to pre-ICS devices.

I should have probably mentioned: we can always replace the default Downloader with something that gives us disk caching on all Android versions (see bug 968897).

	Lucas Rocha (:lucasr)
	Comment 23 • 10 years ago

(In reply to Richard Newman [:rnewman] from comment #15)
> * Metadata (page URL => tile URL) lives in browser.db. Joins, clearing,
> profile support becomes easier.

This is the main reason why I suggested using browser.db for storing metadata btw.

	Wesley Johnston (:wesj) Assignee
	Comment 24 • 10 years ago

Modified this to fit in BrowserProvider. I really hate BrowserProvider and didn't want to add to the mess in it though, so I made this use a little interface between it and the provider. i.e. there's an interface:

interface DatabaseHelper {
    // Defines the url types that this helper can handle
    DatabaseHelpers.ContentType[] getSupportedTypes();

    // called when the db is created to create your tables
    void onCreate(SQLiteDatabase db);
    // Called on updates of the db to update tables
    void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion);

    // Basic content provider calls. These are all actually called in transactions for you.
    Cursor query(SQLiteDatabase db, String[] projection, String selection, String[] selectionArgs, String sortOrder, String groupBy, String limit);
    int update(SQLiteDatabase db, ContentValues values, String selection, String[] selectionArgs);
    long insert(SQLiteDatabase db, ContentValues values);
    int delete(SQLiteDatabase db, String selection, String[] selectionArgs);
};

public class DatabaseHelpers {
    public static class ContentType {
        public final int id; // Number for registering with the url matcher
        public final String name; // Name of the content_type.. I can't remember what that is
        public final String url; // Url suffix to watch for
    }

    public static final DatabaseHelper[] helpers = new DatabaseHelper[] {
        SiteTiles.getInstance()
    };
}

BrowserProvider and BrowserDatabaseHelper both just query the helpers array and call methods on the Helpers. I also want to skip adding more mess to BrowserDB and LocalDb, so basically SiteTiles handles all of the database methods (by implementing a DBHelper), memory cache (holding a LRUCache of recent site), and provides the querying interface finding tiles.

This look ok rnewman? Ideally, I'd like to move all the "bookmarks" or "history" pieces in BrowserProvider out of there into self contained chunks, but for now they'll continue to live on in that mess.

	Richard Newman [:rnewman]
	Comment 25 • 10 years ago

Comment on attachment 8443773 [details] [diff] [review]
WIP v2

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

::: mobile/android/base/GeckoApp.java
@@ +1553,5 @@
>              "Update:Check",
>              "Update:Download",
>              "Update:Install");
>  
> +        SiteTiles.getInstance();

Why isn't this in BrowserApp? (And maybe add a comment that you're initializing something by side-effects.)

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +768,5 @@
>          int offset = createDefaultBookmarks(db, 0);
>  
>          // We'd like to create distribution bookmarks before our own default bookmarks,
>          // but we won't necessarily have loaded the distribution yet. Oh well.
>          enqueueDistributionBookmarkLoad(db, offset);

Your tree is out of date; you're going to get conflicts here.

::: mobile/android/base/db/DatabaseHelpers.java
@@ +10,5 @@
> +import android.database.sqlite.SQLiteDatabase;
> +
> +import android.net.Uri;
> +
> +interface DatabaseHelper {

+1 for trying to decompose BrowserProvider.

I think what you're aiming for here is redefining BrowserProvider in terms of composition -- that is, rather than splitting BrowserProvider into three or more ContentProviders, and needing to decide where stuff like the Combined view lives, you allow a single ContentProvider to delegate to a bunch of composing classes that each share a single database.

However: this name collides fairly hard with SQLiteOpenHelper and BrowserDatabaseHelper. Those are "DatabaseHelper"s in the sense of managing access to a database (in the file/handle sense).

This isn't a helper, and it doesn't manage databases (the CP does via its BrowserDatabaseHelper!); it's a "segment" of a ContentProvider. (Indeed, this interface is basically ContentProvider but with SQLiteDatabase instead of Uri.)


In light of all that, here are some names I think might fit better:

  ContentProvider{Slice,Part,Component,Element}
  {Typed,Sub}DatabaseProvider

@@ +23,5 @@
> +    int delete(SQLiteDatabase db, String selection, String[] selectionArgs);
> +};
> +
> +public class DatabaseHelpers {
> +    public static class ContentType {

I'd put this in DatabaseHelper, so we can eliminate DatabaseHelpers (see below).

@@ +35,5 @@
> +        }
> +    }
> +
> +    public static final DatabaseHelper[] helpers = new DatabaseHelper[] {
> +        SiteTiles.getInstance()

This seems like the wrong place for this. What's wrong with just initializing a list in BrowserProvider#onCreate?

::: mobile/android/base/db/SiteTiles.java
@@ +1,3 @@
> +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,

Nit: your license template is out of date. Should be:

 * This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/.

@@ +40,5 @@
> +        public static final String COLOR = "color";
> +        public static final String IMAGE_URL = "imageUrl";
> +    }
> +
> +    private static final int CACHE_SIZE = 10;

Worth noting why we picked this size.

@@ +82,5 @@
> +
> +    public Info getInfoForUrl(final ContentResolver cr, final String url) {
> +        Info site = cache.get(url);
> +        if (site != null) {
> +            log("cache hit on " + site);

Telemetry?

@@ +151,5 @@
> +    // any time we upgrade
> +    @Override
> +    public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion){
> +        db.execSQL("DROP TABLE IF EXISTS " + Contract.TABLE);
> +        onCreate(db);

Here's a leaky abstraction point.

What we really ought to do here is figure out if *our* schema has changed. There's no point dropping and recreating the table just because we have a migration somewhere else in the DB.

There's going to have to be some conceptual leakage to do that, but I think it's worth adding when we need it. In the mean time this method can be blank.

::: mobile/android/base/home/TopSitesPanel.java
@@ +569,5 @@
>              }
>  
> +            // Suggested images have precedence over thumbnails, no need to wait
> +            // for them to be loaded. See: CursorLoaderCallbacks.onLoadFinished()
> +            SiteTiles.Info info = SiteTiles.getInstance().getInfoForUrl(context.getContentResolver(), url);

I'm not sure this is a good idea (assuming I'm reading this correctly; if I'm not, disregard!).

getInfoForUrl runs a DB query via the CR. We call this for a single URL. That's six queries for our current grid. That can't be good for our throughput.

::: mobile/android/chrome/content/browser.js
@@ +6095,5 @@
> +            }
> +            break;
> +          case "msapplication-TileImage":
> +          case "msapplication-TileColor":
> +            // TODO: Collect all the metadata we can and push it to Java in one go?

File a bug?

	Wesley Johnston (:wesj) Assignee
	Comment 26 • 10 years ago

Further updated database. I renamed the DatabaseHelpers to "Table" since they currently represent tables in the db. We have other things that query more advanced views. I am vaguely hoping "Table" will be able to represent them as well. I made a quick attempt to move history and hit things like [1] where we use the HistoryViewWithFavicons View despite the query being on the "History" content provider URI. I think there's some coupling here we'd have to remove to do it well.

For this new un-coupled things, I still like keeping this separate from BrowserProvider though. :)

I moved the list of Tables into BrowserProvider itself, but also left in an abstract BaseTable implementation that handled some "default" implementations of methods like query/insert/etc (at one point I had two different implementations here, so the BaseTable was nice...). For now only MetadataTable inherits from it.

At one point I had a Metadata object as well, but I gave up and decided maybe it was best to just return a HashMap<String, Object>. I also pulled a lot more tags in browser.js at one point, but it turns out that while there are opengraph/twitter/etc. images available for a lot of sites, many of them aren't what we want for top sites, and others are what we want in the wrong aspect ratio/with no background colors). I decided to leave that to a separate discussion/bug (and force us to do some DB upgrades later).

.. I think thats it. Using these and a LRU cache/Telemetry are in the following patches.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserProvider.java#711

	Wesley Johnston (:wesj) Assignee
	Comment 27 • 10 years ago

This has us use the tile images. I tied this into the current thumbnail to avoid spinning two different loaders. I'm not sure if you'll like that or not lucas?

	Wesley Johnston (:wesj) Assignee
	Comment 28 • 10 years ago

This creates a little LRUCache in MetadataTable to speed lookup. Its set pretty small since we don't show many tiles really. Also adds some telemetry for cache hits.

	Wesley Johnston (:wesj) Assignee
	Comment 29 • 10 years ago

Removed extra file

	Richard Newman [:rnewman]
	Comment 30 • 10 years ago

Comment on attachment 8445521 [details] [diff] [review]
Patch 1/3 - New database

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

N.B., this patch still has Brad as the author.

MetadataTable bothers me. Its DB-accessing methods all take a ContentResolver argument, which makes it like BrowserDB, but it also includes Table functionality. Most of it is static. This doesn't need to have a singleton instance; make a table class, and either hang only static methods off it (no more singleton!), or split off a lightweight accessor class (in parallel with BrowserDB, or embedded in it).

::: mobile/android/base/db/BaseTable.java
@@ +1,4 @@
> +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

Nit: license formatting is wrong.

@@ +13,5 @@
> +
> +// BaseTable provides a basic implementation of a Table for tables that don't require advanced operations during
> +// insert, delete, update, or query operations. Implementors must still provide onCreate and onUpgrade operations.
> +public abstract class BaseTable implements Table {
> +    public static final String LOGTAG = "GeckoBaseTable";

private?

@@ +31,5 @@
> +    }
> +
> +    // Table implementation
> +    @Override
> +    public abstract void onCreate(SQLiteDatabase db);

No need for this, no? This class is abstract.

@@ +35,5 @@
> +    public abstract void onCreate(SQLiteDatabase db);
> +
> +    // Table implementation
> +    @Override
> +    public abstract void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion);

Likewise.

@@ +37,5 @@
> +    // Table implementation
> +    @Override
> +    public abstract void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion);
> +
> +    // Returns the name of the tabel to do modify/query

table

@@ +40,5 @@
> +
> +    // Returns the name of the tabel to do modify/query
> +    protected abstract String getTable();
> +
> +    // Table implementation

No need for all of these comments.

@@ +44,5 @@
> +    // Table implementation
> +    @Override
> +    public Cursor query(SQLiteDatabase db, Uri uri, int dbId, String[] columns, String selection, String[] selectionArgs, String sortOrder, String groupBy, String limit) {
> +        Cursor c = db.query(getTable(), columns, selection, selectionArgs, groupBy, null, sortOrder, limit);
> +        log("query " + columns + " in " + selection + " = " + c);

Better hope Proguard eliminates these when the body of `log` is removed. Sometimes I miss having a proper preprocessor or macros.

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +33,5 @@
>  
>  final class BrowserDatabaseHelper extends SQLiteOpenHelper {
>  
>      private static final String LOGTAG = "GeckoBrowserDBHelper";
> +    public static final int DATABASE_VERSION = 19;

You're racing with Bug 1024289 here, for the record.

@@ +1462,5 @@
>          }
>  
> +        for (Table table : BrowserProvider.getTables()) {
> +            table.onUpgrade(db, oldVersion, newVersion);
> +        }

This is tricky. These table operations need to be performed *after* views are deleted, but *before* they're recreated. This might need reworking.

::: mobile/android/base/db/BrowserProvider.java
@@ +114,5 @@
>      static final Map<String, String> SCHEMA_PROJECTION_MAP;
>      static final Map<String, String> SEARCH_SUGGEST_PROJECTION_MAP;
>      static final Map<String, String> FAVICONS_PROJECTION_MAP;
>      static final Map<String, String> THUMBNAILS_PROJECTION_MAP;
> +    private static final Table[] mTables;

I think you mean sTables (it's static). Or is that kTables (because it's final)? Or maybe just TABLES.

But definitely not mTables, unless you make this non-static. And given that there's only ever one instance of a ContentProvider, you might as well do so!

@@ +250,5 @@
>  
>      // Calculate these once, at initialization. isLoggable is too expensive to
>      // have in-line in each log call.
>      private static boolean logDebug   = Log.isLoggable(LOGTAG, Log.DEBUG);
> +    private static boolean logVerbose = true;//Log.isLoggable(LOGTAG, Log.VERBOSE);

Undo this, and just

  adb shell setprop log.tag.GeckoBrowserProvider VERBOSE

@@ +360,5 @@
>                  return Bookmarks.CONTENT_ITEM_TYPE;
> +            default:
> +                String type = getContentItemType(match);
> +                if (type != null) {
> +                    return type;

Just inline the debugging call below and return null here. You can remove the final return at the end of the method; javac's escape analysis knows this is a complete switch, now.

@@ +452,5 @@
>                  break;
>              }
>  
>              default:
> +                Table table = findTableFor(match);

Brace this block for consistency.

@@ +453,5 @@
>              }
>  
>              default:
> +                Table table = findTableFor(match);
> +                if (table != null) {

if (table == null) {
  throw new Unsu...
}
trace("Deleting table...");
...

@@ +501,5 @@
>              }
>  
>              default:
> +                Table table = findTableFor(match);
> +                if (table != null) {

Same.

@@ +628,5 @@
>              }
>  
>              default:
> +                Table table = findTableFor(match);
> +                if (table != null) {

Same.

@@ +834,5 @@
>              }
>  
>              default:
> +                Table table = findTableFor(match);
> +                if (table != null) {

Same.

@@ +1480,5 @@
>  
>          return results;
>      }
> +
> +    public static Table[] getTables() {

Does this need to be public?

@@ +1495,5 @@
> +        }
> +        return null;
> +    }
> +
> +    private static void addTablesToMatcher(Table[] tables, final UriMatcher matcher) {

Consider simply inlining this. It's called once.

::: mobile/android/base/db/DBUtils.java
@@ +103,5 @@
> +     * as selection args.
> +     * @para columnName   The column to search in
> +     * @para size         The number of arguments to search for
> +     */
> +    public static String buildSelection(String columnName, int size) {

Did you see AbstractTransactionalProvider#computeSQLInClause ?

@@ +109,5 @@
> +        int i = 1;
> +        while (i++ < size) {
> +            selection.append("?, ");
> +        }
> +        selection.append("?)");

This method is buggy if size == 0. (computeSQLInClause is not.)

Crossing module boundary => be careful. Your call in LocalBrowserDB checks its arguments, but some developer six months from now...

::: mobile/android/base/db/LocalBrowserDB.java
@@ +1188,5 @@
>          if (urls == null) {
>              return null;
>          }
>  
> +        final int urlCount = urls.size();

yay final

::: mobile/android/base/db/MetadataTable.java
@@ +22,5 @@
> +import java.util.HashMap;
> +import java.util.HashSet;
> +
> +// Holds metadata info about urls. Supports some helper functions for getting back a HashMap of key value data.
> +public class MetadataTable extends BaseTable {

Can we get "URL" in here somewhere? "URLDescriptionTable", "URLMetadataTable", whatever. MetadataTable kinda implies that this is metadata about the database itself.

@@ +29,5 @@
> +
> +    // Uri for querying this table
> +    private static final Uri CONTENT_URI = Uri.withAppendedPath(BrowserContract.AUTHORITY_URI, "metadata");
> +
> +    public static final String TABLE = "metadata"; // Name of the table in the db

private

@@ +33,5 @@
> +    public static final String TABLE = "metadata"; // Name of the table in the db
> +
> +    // Columns in the table
> +    public static final String ID = "id";
> +    public static final String URL = "url";

This would be clearer later if it were COLUMN_URL or FIELD_URL. (Similar convention for the others, too.)

@@ +37,5 @@
> +    public static final String URL = "url";
> +    public static final String TILE_IMAGE_URL = "tileImage";
> +    public static final String TILE_COLOR = "tileColor";
> +
> +    // This returns a list of columns in the table. Its used to simplify some loops for reading/writing data.

It's

@@ +67,5 @@
> +
> +        HashSet<String> model = getModel();
> +        for (String key : model) {
> +            if (!obj.has(key))
> +                continue;

for (...) {
  if (obj.has(key)) {
    data.put(key, obj.optString(key));
  }
}

@@ +77,5 @@
> +
> +    /**
> +     * Converts a Cursor into a HashMap of known metadata properties.
> +     * Will throw away any properties that aren't stored in the database.
> +     * Will also not iterate through multiple rows int he cursor.

in the

@@ +86,5 @@
> +        HashSet<String> model = getModel();
> +        for (String key : model) {
> +            try {
> +                data.put(key, c.getString(c.getColumnIndexOrThrow(key)));
> +            } catch(Exception ex) {

} catch (Exception ex) {

@@ +87,5 @@
> +        for (String key : model) {
> +            try {
> +                data.put(key, c.getString(c.getColumnIndexOrThrow(key)));
> +            } catch(Exception ex) {
> +                // Its ok not to have every column in our Cursor

Worth logging? This would be a programming error, surely?

@@ +95,5 @@
> +        return data;
> +    }
> +
> +    /**
> +     * Returns a HashMap of url->Metadata (i.e. A second HashMap) for a list of urls.

s/url/URL/g
s/A/a

@@ +96,5 @@
> +    }
> +
> +    /**
> +     * Returns a HashMap of url->Metadata (i.e. A second HashMap) for a list of urls.
> +     * Must not be called from ui or Grecko threads.

UI, Gecko

@@ +105,5 @@
> +        ThreadUtils.assertNotOnUiThread();
> +        ThreadUtils.assertNotOnGeckoThread();
> +
> +        // Nothing to query for
> +        if (urls.size() == 0 || columns.size() == 0) {

isEmpty

@@ +118,5 @@
> +            columns.add(URL);
> +        }
> +
> +        final Cursor cursor = cr.query(CONTENT_URI,
> +                                       columns.toArray(new String[0]), // columns,

new String[columns.size()])

saves an allocation and the need for the comment.

@@ +120,5 @@
> +
> +        final Cursor cursor = cr.query(CONTENT_URI,
> +                                       columns.toArray(new String[0]), // columns,
> +                                       selection, // selection
> +                                       urls.toArray(new String[0]), // selectionargs

Similarly.

@@ +124,5 @@
> +                                       urls.toArray(new String[0]), // selectionargs
> +                                       null);
> +        try {
> +            if (cursor.getCount() < 1) {
> +                data = null;

if (!cursor.moveToFirst()) {
  return null;
}
do {
  ...
} while (cursor.moveToNext());

@@ +143,5 @@
> +
> +    /**
> +     * Saves a HashMap of metadata into the database. Will iterate through columns
> +     * in the Database and only save rows with matching keys in the HashMap.
> +     * Must not be called from ui or Grecko threads.

UI, Gecko

@@ +185,5 @@
> +    @Override
> +    public void onCreate(SQLiteDatabase db) {
> +        StringBuilder create = new StringBuilder("CREATE TABLE " + TABLE + " (");
> +        create.append(ID + " INTEGER PRIMARY KEY, ");
> +        create.append(URL + " STRING NON NULL UNIQUE, ");

TEXT, not STRING

@@ +188,5 @@
> +        create.append(ID + " INTEGER PRIMARY KEY, ");
> +        create.append(URL + " STRING NON NULL UNIQUE, ");
> +        create.append(TILE_IMAGE_URL + " STRING, ");
> +        create.append(TILE_COLOR + " STRING);");
> +        db.execSQL(create.toString());

Just use a big string concatenation for this. The compiler will turn it into a single string, rather than using a StringBuilder at runtime.

@@ +190,5 @@
> +        create.append(TILE_IMAGE_URL + " STRING, ");
> +        create.append(TILE_COLOR + " STRING);");
> +        db.execSQL(create.toString());
> +
> +        db.execSQL("CREATE INDEX metadata_url ON " + TABLE + "(" + URL+ ")");

Spaces around operators, suffix index names with "_idx".

@@ +206,5 @@
> +    // Table implementation.
> +    @Override
> +    public Table.ContentProviderInfo[] getContentProviderInfo() {
> +        return new Table.ContentProviderInfo[] {
> +            new Table.ContentProviderInfo(1200, TABLE)

Magic constant!

::: mobile/android/base/db/Table.java
@@ +1,3 @@
> +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,

Nit: license is wrong.

@@ +9,5 @@
> +import android.database.Cursor;
> +import android.database.sqlite.SQLiteDatabase;
> +import android.net.Uri;
> +
> +// Table's provide a basic wrapper around ContentProvider method's to make it simpler to add new tables into storage.

Tables, methods

@@ +12,5 @@
> +
> +// Table's provide a basic wrapper around ContentProvider method's to make it simpler to add new tables into storage.
> +// If you create a new Table type, make sure to add it to the mTables list in BrowserProvider to ensure it is queried.
> +interface Table {
> +    // Provides information to BrowserProvider about the type of uri's this Table can handle.

URIs

@@ +28,5 @@
> +
> +    // Return a list of Info about the ContentProvider uris this will match
> +    ContentProviderInfo[] getContentProviderInfo();
> +
> +    // Called by BrowserDBHelper whenever the database is created or upgrade

upgraded.

@@ +34,5 @@
> +    // separate table.
> +    void onCreate(SQLiteDatabase db);
> +    void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion);
> +
> +    // Called by BrowserProvider when this database queried/modified

is queried, trailing period.

@@ +35,5 @@
> +    void onCreate(SQLiteDatabase db);
> +    void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion);
> +
> +    // Called by BrowserProvider when this database queried/modified
> +    // The dbId here should match the dbId's you returned in your getContentProviderInfo() call 

Trailing whitespace.

::: mobile/android/chrome/content/browser.js
@@ +3684,5 @@
> +      };
> +    }
> +
> +    if (!this.metatags[type] || this.metatags[type + "_level"] < level) {
> +      Services.console.logStringMessage("WESJ Replace");

*eyebrows* :D

@@ +3725,5 @@
>            type: "DOMContentLoaded",
>            tabID: this.id,
>            bgColor: backgroundColor,
> +          errorType: errorType,
> +          metadata: this.metatags

Trailing comma on last element is currently good style.

@@ +3730,3 @@
>          });
>  
> +        this.metatags = null;

delete this.metatags;

?

@@ +3766,5 @@
>  
> +      case "DOMMetaAdded":
> +        let target = aEvent.originalTarget;
> +        let document = target.ownerDocument;
> +        let browser = BrowserApp.getBrowserForDocument(document);

Inline document.

@@ +3773,5 @@
> +          case "msapplication-TileImage":
> +            this.addMetadata("tileImage", browser.currentURI.resolve(target.content), 10);
> +            break;
> +          case "msapplication-TileColor":
> +            this.addMetadata("tileColor", target.content, 10);

Magic constant appears twice.

	Richard Newman [:rnewman]
	Comment 31 • 10 years ago

Comment on attachment 8445518 [details] [diff] [review]
Patch 3/3 - LRU Cache

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

Needs a couple fixes, and probably needs to be rebased on top of new Part 1, so f+.

::: mobile/android/base/db/MetadataTable.java
@@ +61,5 @@
>  
> +    // Store a cache of recent results. This number is chosen to match the max number of tiles on about:home
> +    private static final int CACHE_SIZE = 9;
> +    private static final LruCache<String, HashMap<String, Object>> cache =
> +        new LruCache<String, HashMap<String, Object>>(CACHE_SIZE);

You don't have strict enough thread safety to use HashMap here if it's mutable. Make this Map<String, Object> and note that members cannot be modified in place.

@@ +125,5 @@
> +            if (hit != null) {
> +                // Cache hit!
> +                data.put(url, hit);
> +            }
> +            Telemetry.HistogramAdd("FENNEC_TILES_CACHE_HIT", data.size());

This should be outside the loop.

@@ +150,5 @@
>                  data = null;
>              } else {
>                  cursor.moveToFirst();
>                  do {
>                      HashMap<String, Object> metadata = fromCursor(cursor);

You might consider having this be an UnmodifiableMap.

	Wesley Johnston (:wesj) Assignee
	Comment 32 • 10 years ago

(In reply to Richard Newman [:rnewman] from comment #30)
> methods off it (no more singleton!), or split off a lightweight accessor
> class (in parallel with BrowserDB, or embedded in it).

I split off an accessor here.

> No need for this, no? This class is abstract.

Declaring the class abstract just means it can't be instantiated. I still need these (or I have to provide stubbed out implementations of them).

> This is tricky. These table operations need to be performed *after* views
> are deleted, but *before* they're recreated. This might need reworking.

Yeah. This is tricky... but I regret nothing.

> Did you see AbstractTransactionalProvider#computeSQLInClause ?

I moved this (and its buddy) into DBUtils, because I'm friends DBUtils. You ok with that?

> > +        this.metatags = null;
> delete this.metatags;

One of the devtools guys just recently told me that "delete" distorts an object's shape, which throws us into slow paths. His advice was to just never use it, so I've stopped. :)

	Richard Newman [:rnewman]
	Comment 33 • 10 years ago

Comment on attachment 8447348 [details] [diff] [review]
Patch 1/3 - New database

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

Wes: did you forget to `hg add URLMetadata.java`?

> I moved this (and its buddy) into DBUtils, because I'm friends DBUtils. You ok with that?

Yes!

> One of the devtools guys just recently told me that "delete" distorts an object's
> shape, which throws us into slow paths. His advice was to just never use it, so
> I've stopped. :)

Good to know! Thanks for sharing why.

::: mobile/android/base/Tab.java
@@ +312,5 @@
> +        final HashMap<String, Object> data = URLMetadata.fromJSON(metadata);
> +        ThreadUtils.postToBackgroundThread(new Runnable() {
> +            @Override
> +            public void run() {
> +                URLMetadata.save(mAppContext.getContentResolver(), data);

Let's capture mAppContext (or the ContentResolver itself) in a final local outside the Runnable.

I know this anonymous class will capture the Tab, but it's generally a scary practice to refer to a mutable member in the enclosing instance both at a later instant in time and from a different thread.

::: mobile/android/base/db/BaseTable.java
@@ +29,5 @@
> +    public Table.ContentProviderInfo[] getContentProviderInfo() {
> +        return new Table.ContentProviderInfo[0];
> +    }
> +
> +    // Table implementation

I still think these comments can go :)

Move all of the Table @Overrides into one section if that helps.

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +1727,5 @@
>          }
>  
> +        for (Table table : BrowserProvider.getTables()) {
> +            table.onUpgrade(db, oldVersion, newVersion);
> +        }

Newline after this }.

::: mobile/android/base/db/BrowserProvider.java
@@ +121,4 @@
>  
>      static {
> +        sTables = new Table[] {
> +            URLMetadataTable.getInstance(),

Just 

  new URLMetadataTable();

see singleton comment in that file.

@@ +1495,5 @@
>      }
> +
> +    static Table[] getTables() {
> +        return sTables;
> +    }

Not called and not public. Can this die?

@@ +1509,5 @@
> +        return null;
> +    }
> +
> +    private static void addTablesToMatcher(Table[] tables, final UriMatcher matcher) {
> +    }

Not called and private -- can this die?

::: mobile/android/base/db/Table.java
@@ +1,4 @@
> +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

Nit: same ol' license thing.

@@ +21,5 @@
> +                                  // BrowserProvider will return "vnd.android.cursor.item/" + name
> +
> +        public ContentProviderInfo(int id, String name) {
> +            this.id = id;
> +            this.name = name;

Safety nit: throw here if either is null?

@@ +29,5 @@
> +    // Return a list of Info about the ContentProvider URIs this will match
> +    ContentProviderInfo[] getContentProviderInfo();
> +
> +    // Called by BrowserDBHelper whenever the database is created or upgraded.
> +    // Order of those calls isn't guaranteed (yet), so be careful if your Table depends on something in a

Order of calls to each table...

The way this is worded sounds like we might call onUpgrade then onCreate!

::: mobile/android/base/db/URLMetadataTable.java
@@ +1,5 @@
> +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + */

Nit: newline after license.

@@ +12,5 @@
> +import org.json.JSONObject;
> +
> +import android.content.ContentValues;
> +import android.content.ContentResolver;
> +import android.database.Cursor;

Remove unused imports.

@@ +24,5 @@
> +
> +// Holds metadata info about urls. Supports some helper functions for getting back a HashMap of key value data.
> +public class URLMetadataTable extends BaseTable {
> +    private static final String LOGTAG = "GeckoURLMetadataTable";
> +    private static URLMetadataTable sInstance;

You can kill the singleton. ContentProvider is the "super-singleton".

@@ +67,5 @@
> +    }
> +
> +    @Override
> +    public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion) {
> +        // This table was addedin v19 of the db. Force its creation if we're coming from an earlier version

added in

@@ +68,5 @@
> +
> +    @Override
> +    public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion) {
> +        // This table was addedin v19 of the db. Force its creation if we're coming from an earlier version
> +        if (newVersion == 19 && oldVersion < 19) {

This should be >= 19, not == 19. You only call onUpgrade once; the step-by-step part of BrowserDatabaseHelper doesn't apply here.

	Lucas Rocha (:lucasr)
	Comment 34 • 10 years ago

Comment on attachment 8445517 [details] [diff] [review]
Patch 2/3 - Use tile images

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

This is looking pretty nice. Still needs some cleanups.

A couple of weeks ago, mfinkle and I discussed a bit about how we could provide some sort of add-on API to allow them to manually set nice images for specific URLs. I wonder if this new metadata table could how we store those? The API could be something like:

SiteMetadata.put("https://example.com/", SiteMetadata.IMAGE_URL, "https://example.com/image.jpg");
SiteMetadata.put("https://example.com/", SiteMetadata.BG_COLOR, "#ffcc00");

...and...

SiteMetadata.remove("https://example.com");
SiteMetadata.remove("https://example.com", SiteMetadata.IMAGE_URL);

Or maybe this could be something like a pagemod add-on that injects metadata to pages with specific URLs?

::: mobile/android/base/home/TopSitesGridItemView.java
@@ +171,5 @@
>  
>          if (thumbnail != null) {
> +            if (thumbnail.bitmapUrl != null) {
> +                Log.i(LOGTAG, "Use metadata for " + thumbnail.bitmapUrl);
> +                displayThumbnail(thumbnail.bitmapUrl, thumbnail.color);

You probably want to check what happens if the page has both a suggested site image & metadata image. For instance, you want to avoid triggered a redundant Picasso request. Not sure which one should take precedence. The suggest image maybe?

::: mobile/android/base/home/TopSitesPanel.java
@@ +695,5 @@
> +        public final int color;
> +
> +        public ThumbnailInfo(Bitmap bitmap) {
> +            this.bitmap = bitmap;
> +            this.bitmapUrl = null;

nit: maybe 'imageUrl' instead?

@@ +696,5 @@
> +
> +        public ThumbnailInfo(Bitmap bitmap) {
> +            this.bitmap = bitmap;
> +            this.bitmapUrl = null;
> +            this.color = Color.TRANSPARENT;

nit: maybe 'bgColor' instead?

@@ +711,5 @@
> +            if (imageUrl == null) {
> +                return null;
> +            }
> +
> +            int color = -1;

nit: this can be:

final int color;

@@ +715,5 @@
> +            int color = -1;
> +            String colorString = (String) data.get(MetadataTable.TILE_COLOR);
> +            try {
> +                color = Color.parseColor(colorString);
> +            } catch(Exception ex) {

nit: space after catch

@@ +716,5 @@
> +            String colorString = (String) data.get(MetadataTable.TILE_COLOR);
> +            try {
> +                color = Color.parseColor(colorString);
> +            } catch(Exception ex) {
> +                color = Color.TRANSPARENT;

Is TRANSPARENT a sane enough default here? Maybe white is safer? Maybe get the dominant color just like favicons?

@@ +727,5 @@
>      /**
>       * An AsyncTaskLoader to load the thumbnails from a cursor.
>       */
> +    private static class ThumbnailsLoader extends AsyncTaskLoader<Map<String, ThumbnailInfo>> {
> +        private Map<String, ThumbnailInfo> mThumbnailInfo;

nit: mThumbnailInfo in singular form is a bit misleading. mThumbnailsInfo or mThumbnailInfos?

@@ +746,5 @@
>              final ContentResolver cr = getContext().getContentResolver();
>              final Cursor cursor = BrowserDB.getThumbnailsForUrls(cr, mUrls);
> +            final ArrayList<String> columns = new ArrayList<String>();
> +            columns.add(MetadataTable.TILE_IMAGE_URL);
> +            columns.add(MetadataTable.TILE_COLOR);

The columns can be statically defined in the class, no?

@@ +747,5 @@
>              final Cursor cursor = BrowserDB.getThumbnailsForUrls(cr, mUrls);
> +            final ArrayList<String> columns = new ArrayList<String>();
> +            columns.add(MetadataTable.TILE_IMAGE_URL);
> +            columns.add(MetadataTable.TILE_COLOR);
> +            final HashMap<String, HashMap<String, Object>> metadata = MetadataTable.getForUrls(cr, mUrls, columns);

Given that you're giving the metadata images precedence over the thumbnails, maybe you should first load metadata for all input urls and and only query thumbnails for the urls you haven't found metadata for. This will save us a bit of time querying unnecessary bitmaps from the DB.

Are we giving up the whole BrowserDB layer encapsulating the specific tables? I haven't followed the other patches but I'd expect something like a BrowserDB.getMetadataForUrls() here to be consistent?

	Wesley Johnston (:wesj) Assignee
	Comment 36 • 10 years ago

(In reply to Lucas Rocha (:lucasr) from comment #34)
> Are we giving up the whole BrowserDB layer encapsulating the specific
> tables? I haven't followed the other patches but I'd expect something like a
> BrowserDB.getMetadataForUrls() here to be consistent?

BrowserDB just feels a bit cluttered to me, so instead I'm using a separate URLMetadata class to handle access here. I wonder if it would be some nice cleanup to have History, Bookmarks, ReadingList etc. classes as well (and kill off the system bookmarks support). Or maybe that's just moving code around for nothing.

Thoughts? Disagree?

	Richard Newman [:rnewman]
	Comment 37 • 10 years ago

Comment on attachment 8449735 [details] [diff] [review]
Patch 1/3 - New database

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

I don't see the switch from 18 to 19:

-    public static final int DATABASE_VERSION = 18;
+    public static final int DATABASE_VERSION = 19;

Did you rebase on top of eeden's backed-out patch? If so, you need to jump to 20, and wait for him to reland. If not, please make sure the DATABASE_VERSION change comes back!

::: mobile/android/base/db/Table.java
@@ +10,5 @@
> +import android.database.sqlite.SQLiteDatabase;
> +import android.net.Uri;
> +
> +// Tables provide a basic wrapper around ContentProvider methods to make it simpler to add new tables into storage.
> +// If you create a new Table type, make sure to add it to the mTables list in BrowserProvider to ensure it is queried.

s/mTables/sTables.

	Richard Newman [:rnewman]
	Comment 38 • 10 years ago

> Thoughts? Disagree?

Thumbs up from me.

	Wesley Johnston (:wesj) Assignee
	Comment 39 • 10 years ago

Err apologies for the churn. This compiles!

	Lucas Rocha (:lucasr)
	Comment 40 • 10 years ago

(In reply to Wesley Johnston (:wesj) from comment #36)
> (In reply to Lucas Rocha (:lucasr) from comment #34)
> > Are we giving up the whole BrowserDB layer encapsulating the specific
> > tables? I haven't followed the other patches but I'd expect something like a
> > BrowserDB.getMetadataForUrls() here to be consistent?
> 
> BrowserDB just feels a bit cluttered to me, so instead I'm using a separate
> URLMetadata class to handle access here. I wonder if it would be some nice
> cleanup to have History, Bookmarks, ReadingList etc. classes as well (and
> kill off the system bookmarks support). Or maybe that's just moving code
> around for nothing.
> 
> Thoughts? Disagree?

Fair enough. Go for it.

	Richard Newman [:rnewman]
	Comment 41 • 10 years ago

Comment on attachment 8449760 [details] [diff] [review]
Patch 1/3 - New database

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

Needs a new version with the DB version change. Knocking this off my queue for now.

	Wesley Johnston (:wesj) Assignee
	Comment 42 • 10 years ago

Changed two characters.

	Richard Newman [:rnewman]
	Comment 44 • 10 years ago

Comment on attachment 8450434 [details] [diff] [review]
Patch 1/3 - New database

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

Assuming that this works on device and tests pass, rock on!

	Wesley Johnston (:wesj) Assignee
	Comment 45 • 10 years ago

Waiting on some other db stuff before landing the table. This is updated to address most comments. I spent a little while trying to extract colors if we don't have them, but decided to punt for now. Its tough to get the Bitmap from Picasso. Even passing a Callback to the into() method, it didn't seem to have been attached to the ImageView itself yet.

	Wesley Johnston (:wesj) Assignee
	Comment 46 • 10 years ago

I'd never played with unmodifiableMap. I went a bit nuts with it and made everything unmodifiable. That might make it hard if you want to do something like:

Map data = URLMetadata.get();
data.put(x, y);
UrlMetadata.save(data);

I guess instead you'd have to do:

Map data = new Hashmap();
Collections.copy(URLMetadata.get(), data);
data.put(x, y);
UrlMetadata.save(data);

I can live with that I think...

	Richard Newman [:rnewman]
	Comment 47 • 10 years ago

Comment on attachment 8452579 [details] [diff] [review]
Patch 3/3 - LRU Cache

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

I suggested unmodifiable in this case because we don't mutate these things *now*, so making them unmodifiable helps ensure thread-safety. If we switched into a mutation-heavy approach (as we have with the favicon cache, for example), we'd probably revisit and use ConcurrentHashMap or similar. But for now, going hog-wild on unmodifiableMap is awesome :D

r+ with fixes inline.

::: mobile/android/base/db/URLMetadata.java
@@ +140,1 @@
>                  data.put(url, metadata);

There's some needless work being done here.

Let's say `urls` contains 10 items. 9 of them are in the cache, so we already put those into `data`.

We don't discard the ones that matched, so line 130 passes them all in. We query for all ten from the DB, then walk through them here re-adding them to the cache and the output.

I contend that we want an `else` on line 111 that collects the non-matching URLs. 117 checks if that new collection is empty. 121-130 uses the new collection instead of `urls`, and then here we do no extra work.

::: toolkit/components/telemetry/Histograms.json
@@ +6472,5 @@
> +  "FENNEC_TILES_CACHE_HIT": {
> +    "expires_in_version": "never",
> +    "kind": "linear",
> +    "high": "9",
> +    "n_buckets": 9,

It's common to pad the telemetry count to allow for future extension -- will we ever allow 12 tiles on a TV or a tablet? Any harm in setting this to higher than 9?

	Lucas Rocha (:lucasr)
	Comment 48 • 10 years ago

Comment on attachment 8452559 [details] [diff] [review]
Patch 2/3 - Use tile images

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

Looks good with the suggested changes. Feel free to request another review if you end up changing more than a few things in the patch.

::: mobile/android/base/Tab.java
@@ +313,5 @@
>          final HashMap<String, Object> data = URLMetadata.fromJSON(metadata);
>          ThreadUtils.postToBackgroundThread(new Runnable() {
>              @Override
>              public void run() {
> +                URLMetadata.save(cr, mUrl, data);

Seems like this change belongs to the previous patch that introduces URLMetadata.

::: mobile/android/base/db/URLMetadata.java
@@ +125,5 @@
>       * Saves a HashMap of metadata into the database. Will iterate through columns
>       * in the Database and only save rows with matching keys in the HashMap.
>       * Must not be called from UI or Gecko threads.
>       */
> +    public static void save(final ContentResolver cr, final String url, final HashMap<String, Object> data) {

Same here, this change belongs to the previous patch, it seems,

::: mobile/android/base/gfx/BitmapUtils.java
@@ +121,5 @@
>  
>              try {
>                  final Drawable d = context.getPackageManager().getApplicationIcon(resource);
>                  runOnBitmapFoundOnUiThread(loader, d);
> +            } catch (Exception ex) { }

Unrelated?

::: mobile/android/base/home/TopSitesPanel.java
@@ +58,5 @@
>  import android.view.ViewGroup;
>  import android.widget.AdapterView;
>  import android.widget.ListView;
>  
> +import static android.graphics.Color.WHITE;

bit: I'd use Color.WHITE in the code just for extra clarity.

@@ +694,5 @@
>              }
>          }
>      }
>  
> +    static class ThumbnailInfo {

private?

@@ +702,5 @@
> +
> +        public ThumbnailInfo(final Bitmap bitmap) {
> +            this.bitmap = bitmap;
> +            this.imageUrl = null;
> +            this.bgColor = WHITE;

I'd go with transparent here as the thumbnail fills the whole area anyway.

@@ +742,5 @@
>          }
>  
>          @Override
> +        public Map<String, ThumbnailInfo> loadInBackground() {
> +            final Map<String, ThumbnailInfo> thumbnails = new HashMap<String, ThumbnailInfo>();

This changes the return value from null to empty map if no urls were provided. Intentional?

@@ +751,5 @@
>              // Query the DB for thumbnails.
>              final ContentResolver cr = getContext().getContentResolver();
> +            final ArrayList<String> columns = new ArrayList<String>();
> +            columns.add(TILE_IMAGE_URL_COLUMN);
> +            columns.add(TILE_COLOR_COLUMN);

No need to create this column list on every loader execution. Create once statically and reuse it on all executions.

@@ +752,5 @@
>              final ContentResolver cr = getContext().getContentResolver();
> +            final ArrayList<String> columns = new ArrayList<String>();
> +            columns.add(TILE_IMAGE_URL_COLUMN);
> +            columns.add(TILE_COLOR_COLUMN);
> +            final HashMap<String, HashMap<String, Object>> metadata = URLMetadata.getForUrls(cr, mUrls, columns);

Follow-up suggestion: wrap this verbose map of maps into something nicer to read e.g. Map<String, Metadata>. Not a big deal though.

nit: use the interface instead: Map<String, Map<String, Object>>.

@@ +757,4 @@
>  
> +            if (metadata != null) {
> +                for (String url : metadata.keySet()) {
> +                    ThumbnailInfo info = ThumbnailInfo.fromMetadata(metadata.get(url));

fromMetadata() could still return null here, no? I'd probably to a null check here just in case. Something like:

if (info == null) {
    continue;
}

@@ +760,5 @@
> +                    ThumbnailInfo info = ThumbnailInfo.fromMetadata(metadata.get(url));
> +                    thumbnails.put(url, info);
> +
> +                    // Remove any url's we found metadata for from the thumbnail search.
> +                    mUrls.remove(url);

This is a bit expensive to do as it involves a linear search in the list. Also, this changes the input list of URLs which was meant to be unmodifiable to avoid confusion. So I'd personally prefer this to be implemented with a local list with the urls you haven't found metadata for.

I don't feel strongly about it. Up to you.

	Wesley Johnston (:wesj) Assignee
	Comment 49 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/0814bb0f08d0

	Carsten Book [:Tomcat]
	Comment 50 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/0814bb0f08d0