The topsites view at the top of the mobile AS can probably be implemented as an independent View (that view can then be plugged into the multi row layout that's used for the AS panel itself). The design isn't finalised yet, for now a horizontal scrollable list of cards is probably acceptable (the appearance/behaviour can be tweaked later as needed). For the data we can use the existing topsites query, and perhaps add a parameter to allow disabling pinned sites (in future, once the design is more firm, a separate query would probably be more efficient - the current query has to do a lot of additional work to get pinning and ordering right).

Here's a screen of my local patch. The (currently black) background should match the favicons dominant colour, the titles also don't seem ideal - but at least we have a framework for showing topsites. (The menu is currently non-functional.)

Comment on attachment 8779524 [details] Bug 1293790 - WIP: Implement Paged TopSites View https://reviewboard.mozilla.org/r/70504/#review69174 ::: mobile/android/base/resources/layout/as_topsites_card.xml:1 (Diff revision 1) > +<?xml version="1.0" encoding="utf-8"?> We probably want to rename this to activity_stream_topsites_card ?

Comment on attachment 8779523 [details] Bug 1293790 - Pre: Extract UpdateViewFaviconLoadedListener to allow reuse https://reviewboard.mozilla.org/r/70502/#review69210 ::: mobile/android/base/java/org/mozilla/gecko/home/UpdateViewFaviconLoadedListener.java:1 (Diff revision 2) > +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*- I need to add this to moz.build

Comment on attachment 8779524 [details] Bug 1293790 - WIP: Implement Paged TopSites View https://reviewboard.mozilla.org/r/70504/#review69212 ::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/topsites/TopSitesAdapter.java:1 (Diff revision 2) > +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*- All of these need to be added to moz.build

Comment on attachment 8779521 [details] Bug 1293790 - Implement getPlainTopSites query https://reviewboard.mozilla.org/r/70498/#review69592 To be honest these queries are a bit hard to follow; I'll trust that this does what you're intending. I know that designs aren't finalized... But, will new top sites row won't have anything other than "top sites" mixed in? Will pinned sites be a separate row? Might we have a separate "suggested sites" row? If the answer to these is a resounding "yes", this stuff should be broken down nicely into cleaner, simple queries. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:947 (Diff revision 2) > suggestedLimitClause + " )", > > suggestedSiteArgs); > } > > if (hasPreparedBlankTiles) { Wouldn't it be the same (but cleaner) to do something like this instead? And not bother changing `blanksLimitClause`. `if (hasPreparedBlankTiles && pinnedSitesEnabled) { ... }`

Comment on attachment 8779522 [details] Bug 1293790 - Pre: implement AS topsites access https://reviewboard.mozilla.org/r/70500/#review69598 I understand that this is a WIP, but I think we should build this stuff with performance telemetry in mind. ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1849 (Diff revision 2) > > urls.add(url); > } while (c.moveToNext()); > } > > + public CursorLoader getActivityStreamTopSites(Context context, int limit) { Can we still measure/report via telemetry how long this query will take, if we let CursorLoader handle the Cursor behind the curtains? Currently in TopSitesPanel we measure this, and we shouldn't loose the capability and those performance data points. ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1871 (Diff revision 2) > + null, > + null); > + } > + > @Override > public Cursor getTopSites(ContentResolver cr, int suggestedRangeLimit, int limit) { We should revisit these methods once designs are finalized. Ideally we should end up with just a few simple queries.

Comment on attachment 8779523 [details] Bug 1293790 - Pre: Extract UpdateViewFaviconLoadedListener to allow reuse https://reviewboard.mozilla.org/r/70502/#review69792 Oh yeah, while working on fixing and refactoring the favicon code I saw a class like this used in a lot of places. :)

Comment on attachment 8779524 [details] Bug 1293790 - WIP: Implement Paged TopSites View https://reviewboard.mozilla.org/r/70504/#review69794 ::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/topsites/TopSitesAdapter.java:16 (Diff revision 2) > +import android.view.ViewGroup; > + > +import org.mozilla.gecko.R; > +import org.mozilla.gecko.db.BrowserContract; > + > +public class TopSitesAdapter extends RecyclerView.Adapter<TopSitesCard> { This adapter is backed by a Cursor, so it should have stable ids. A bunch of optimizations kick in if you implement those methods additionally: * hasStableIds() * getItemId() ::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/topsites/TopSitesAdapter.java:23 (Diff revision 2) > + > + @Override > + public TopSitesCard onCreateViewHolder(ViewGroup parent, int viewType) { > + final LayoutInflater inflater = LayoutInflater.from(parent.getContext()); > + > + final CardView card = (CardView) inflater.inflate(R.layout.as_topsites_card, parent, false); The "as" prefix is still confusing me. I read this as "layout as topsites card" :) ::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/topsites/TopSitesAdapter.java:44 (Diff revision 2) > + return 0; > + } > + } > + > + public void swapCursor(Cursor cursor) { > + this.cursor = cursor; I think the CursorAdapter of the framework closes the old cursor when swapping in a new one. Is closing the old cursor handled somewhere else? ::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/topsites/TopSitesAdapter.java:47 (Diff revision 2) > + public String getURLForPosition(int position) { > + cursor.moveToPosition(position); > + > + return cursor.getString(cursor.getColumnIndexOrThrow(BrowserContract.Combined.URL)); > + } This method should be annotated with @UIThread (Or maybe even add a ThreadUtils.assert()). RecyclerView will access the other methods on the UIThread. If someone else moves the cursor on a different thread then this can cause some concurrency issues. ::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/topsites/TopSitesCard.java:32 (Diff revision 2) > + private final TextView title; > + private final View menuButton; > + > + private final UpdateViewFaviconLoadedListener mFaviconListener; > + > + private String url = ""; Do we need to initialize the url with an empty string? In this version of the code it doesn't seem to be used outside of bind() anyways? ::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/topsites/TopSitesCard.java:47 (Diff revision 2) > + title = (TextView) card.findViewById(R.id.title); > + menuButton = card.findViewById(R.id.menu); > + > + // Disable corners on < lollipop: > + // CardView only supports clipping content on API >= 21 (for performance reasons). Without > + // content clipping the "action bar" will look ugly because it has its own background: "action bar"? :) ::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/topsites/TopSitesCursorLoaderCallbacks.java:33 (Diff revision 2) > + final Context context = contextWeakRef.get(); > + if (context == null) { > + return null; > + } > + > + return GeckoProfile.get(context).getDB().getActivityStreamTopSites(context, TopSitesView.TOPSITES_LIMIT); I'm confused: Why does the Browser DB return a Cursor *Loader*? Do we use this pattern somewhere else? ::: mobile/android/base/resources/layout/as_topsites_card.xml:1 (Diff revision 2) > +<?xml version="1.0" encoding="utf-8"?> I propose that for new layout files we use the default formatting rules of Android (Studio) [Code->Reformat Code, Code->Rearange code]. With the current formatting the structure is hard to identify and you constantly have to fight the IDE to format the XML file like this.

Comment on attachment 8779524 [details] Bug 1293790 - WIP: Implement Paged TopSites View https://reviewboard.mozilla.org/r/70504/#review69802

Comment on attachment 8782116 [details] Bug 1293790 - Pre: extract CardView corner workaround into FilledCardView https://reviewboard.mozilla.org/r/72342/#review70188

Comment on attachment 8779521 [details] Bug 1293790 - Implement getPlainTopSites query (re-requesting review because the patch has changed significantly)

Comment on attachment 8779521 [details] Bug 1293790 - Implement getPlainTopSites query https://reviewboard.mozilla.org/r/70498/#review70836 ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:749 (Diff revision 3) > + * will be appended (if necessary) to the end of the list in order to provide up to PARAM_LIMIT items. > + */ > + private Cursor getPlainTopSites(final Uri uri) { > + final SQLiteDatabase db = getReadableDatabase(uri); > + > + final String limitParam = uri.getQueryParameter(BrowserContract.PARAM_LIMIT); Consider adding a null check and a default value here; callers might omit limit param in the uri and this will throw (although right now it's just one caller). ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:758 (Diff revision 3) > + // and about: pages. > + final String ignoreForTopSitesWhereClause = > + "(" + Combined.HISTORY_ID + " IS NOT -1)" + > + " AND " + > + Combined.URL + " NOT IN (SELECT " + > + Bookmarks.URL + " FROM bookmarks WHERE " + There's TABLE_BOOKMARKS, if you care to use it. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:768 (Diff revision 3) > + > + final String[] ignoreForTopSitesArgs = new String[] { > + AboutPages.URL_FILTER > + }; > + > + final Cursor c = db.rawQuery(" SELECT " + extra space? ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:787 (Diff revision 3) > + if (c.getCount() == limit) { > + return c; > + } > + > + // If we don't have enough data: get suggested sites too > + final SuggestedSites suggestedSites = GeckoProfile.get(getContext(), uri.getQueryParameter(BrowserContract.PARAM_PROFILE)).getDB().getSuggestedSites(); I typed out text below, and then realized that you're wrapping this in a CursorLoader in LocalBrowserProvider... If there are good reasons to return CursorLoader out of the LocalBrowserProvider method which consumes this, ignore the comment :-) But it does feel out of place in that it doesn't follow our regular pattern. "I think unless you're going to do some "stuff suggested sites into sql query" tricks a la getTopSites does, this is worth just lifting to LocalBrowserProvider, and simplifying this method to really _just_ return top sites. You can easily do the merge upstream if you need to." ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:948 (Diff revision 3) > // Pinned site positions are zero indexed, but we need to get the maximum 1-indexed position. > // Hence to correctly calculate the largest pinned position (which should be 0 if there are > // no sites, or 1-6 if we have at least one pinned site), we coalesce the DB position (0-5) > // with -1 to represent no-sites, which allows us to directly add 1 to obtain the expected value > // regardless of whether a position was actually retrieved. > - final String blanksLimitClause = " LIMIT MAX(0, " + > + final String blanksLimitClause; why?

Comment on attachment 8779522 [details] Bug 1293790 - Pre: implement AS topsites access https://reviewboard.mozilla.org/r/70500/#review70842 ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1838 (Diff revision 3) > + > + /** > + * Internal CursorLoader that extends the framework CursorLoader in order to measure > + * performance for telemetry purposes. > + */ > + private final class TelemtrisedCursorLoader extends CursorLoader { Probably want to make this `static`. ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1841 (Diff revision 3) > + * performance for telemetry purposes. > + */ > + private final class TelemtrisedCursorLoader extends CursorLoader { > + final String mHistogramName; > + > + public TelemtrisedCursorLoader(Context context, Uri uri, String[] projection, String selection, Spelling. ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1866 (Diff revision 3) > + > + public CursorLoader getActivityStreamTopSites(Context context, int limit) { > + final Uri uri = mTopSitesUriWithProfile.buildUpon() > + .appendQueryParameter(BrowserContract.PARAM_LIMIT, > + String.valueOf(limit)) > + .appendQueryParameter(BrowserContract.PARAM_SUGGESTEDSITES_LIMIT, This param is ignored in BrowserProvider method. ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1871 (Diff revision 3) > + .appendQueryParameter(BrowserContract.PARAM_SUGGESTEDSITES_LIMIT, > + String.valueOf(limit)) > + .appendQueryParameter(BrowserContract.PARAM_TOPSITES_DISABLE_PINNED, Boolean.TRUE.toString()) > + .build(); > + > + return new TelemtrisedCursorLoader(context, Or, you could return a simple Cursor here and wrap it in a CursorLoader in your TopSitesCursorLoaderCallbacks class?

Comment on attachment 8779522 [details] Bug 1293790 - Pre: implement AS topsites access https://reviewboard.mozilla.org/r/70500/#review70846 Please consider moving CursorLoader stuff outside of LocalBrowserProvider and into your CursorLoaderCallbacks class, and then you'll be able to lift merging of suggested sites into LocalBrowserProvider. Otherwise, we're poorly mixing concerns here.

Comment on attachment 8779522 [details] Bug 1293790 - Pre: implement AS topsites access https://reviewboard.mozilla.org/r/70500/#review70842 > Or, you could return a simple Cursor here and wrap it in a CursorLoader in your TopSitesCursorLoaderCallbacks class? Based on bug 1239491 we're trying to move away from writing our own CursorLoader implementations: the framework CursorLoader seems to be designed to directly replace ContentResolver.query(): I.e. old method: - Implement getFoo() in BrowserDB, it returns a new cursor - Extend SimpleCursorLoader (deprecated) to create FooLoader, it uses db.getFoo() when queries are necessary - Create new FooLoader in CursorLoaderCallbacks when a loader is requested New method: - Implement getFoo in BrowserDB, it returns a (framework) CursorLoader - Return that CursorLoader from CursorLoaderCallbacks when a loader is requested (Framework CursorLoader performs the query when asked - and we don't have to write this wrapper class.)

(In reply to Andrzej Hunt :ahunt from comment #28) > > Or, you could return a simple Cursor here and wrap it in a CursorLoader in your TopSitesCursorLoaderCallbacks class? > > Based on bug 1239491 we're trying to move away from writing our own > CursorLoader implementations: the framework CursorLoader seems to be > designed to directly replace ContentResolver.query(): Just to add: we'd still need to wrap the framework CursorLoader to add telemetry - however we're able to avoid implementing the db querying code, and any other niceties that the framework CursorLoader offers (i.e. handling load cancellation).

I've taken a first stab at moving to the paged version of topsites. The patches provide a fairly configurable setup (we can tune the number of items in terms of width, and we can also switch to a two-row setup if appropriate). There are still quite a few things remaining: - Pager Indicator: this might require importing a library [1] - Background colour for generated favicons (we currently show a white background in the FaviconView) - Tests for the URL extraction code, if we use that - Better menu icon - Implementing the menu (I don't think the menu is spec'd yet). - Click support (mostly done, but not hooked up because that will become simpler when we move back into the HomePager). (... more?) [1] https://github.com/JakeWharton/ViewPagerIndicator/blob/master/library/src/com/viewpagerindicator/CirclePageIndicator.java

Comment on attachment 8779522 [details] Bug 1293790 - Pre: implement AS topsites access https://reviewboard.mozilla.org/r/70500/#review71814 Thanks for clarifying the CursorLoader pattern. Looks good!

Comment on attachment 8784476 [details] Bug 1293790 - Import CirclePageIndicator from ViewPagerIndicator library https://reviewboard.mozilla.org/r/73918/#review72086 ::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/topsites/PageIndicator.java:26 (Diff revision 1) > + > +/** > + * A PageIndicator is responsible to show an visual indicator on the total views > + * number and the current visible view. > + */ > +public interface PageIndicator extends ViewPager.OnPageChangeListener { If we are just importing CirclePageIndicator maybe we can ditch the interface? ::: mobile/android/base/resources/values/vpi__attrs.xml:18 (Diff revision 1) > + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > + See the License for the specific language governing permissions and > + limitations under the License. > +--> > + > +<resources> I saw that you started to customize the CirclePageIndicator implementation later. Maybe we should merge the attrs and values into our xml files? As long as lint does a good job at indentifying unused ones this shouldn't be a big problem, I guess.

Comment on attachment 8784477 [details] Bug 1293790 - Increase separation between circles in indicator https://reviewboard.mozilla.org/r/73920/#review72088

Comment on attachment 8784478 [details] Bug 1293790 - Add indicator to topsites pager https://reviewboard.mozilla.org/r/73922/#review72090

https://hg.mozilla.org/integration/fx-team/rev/0a73bdb24cce8b33b65fa1ad2df14f5040e49b7c Bug 1293790 - Implement getPlainTopSites query r=grisha https://hg.mozilla.org/integration/fx-team/rev/7d4242f2976d02d0f04a5b9e95d3f30b05bfc830 Bug 1293790 - Pre: implement AS topsites access r=grisha https://hg.mozilla.org/integration/fx-team/rev/d2ec2e06bbdf6d7e9e3d7cd5c58679684f66b800 Bug 1293790 - Pre: extract CardView corner workaround into FilledCardView r=sebastian https://hg.mozilla.org/integration/fx-team/rev/4be070f03d4761aa5d06a8686b4daaef4213b5c1 Bug 1293790 - Pre: Extract UpdateViewFaviconLoadedListener to allow reuse r=sebastian https://hg.mozilla.org/integration/fx-team/rev/326bcca7b6b6c7a3a9e3c2c6bb87f3385c92867a Bug 1293790 - Implement Paged TopSites View r=sebastian https://hg.mozilla.org/integration/fx-team/rev/048e2f46f7436a8623257b90178e4a96bf390b67 Bug 1293790 - Import CirclePageIndicator from ViewPagerIndicator library r=sebastian https://hg.mozilla.org/integration/fx-team/rev/a0d138d05c26ca791bc85be220e60ec8adce14c7 Bug 1293790 - Increase separation between circles in indicator r=sebastian https://hg.mozilla.org/integration/fx-team/rev/9ff5f0b2f4e0720fe55209efe86c40c0a63787ab Bug 1293790 - Add indicator to topsites pager r=sebastian

https://hg.mozilla.org/mozilla-central/rev/0a73bdb24cce https://hg.mozilla.org/mozilla-central/rev/7d4242f2976d https://hg.mozilla.org/mozilla-central/rev/d2ec2e06bbdf https://hg.mozilla.org/mozilla-central/rev/4be070f03d47 https://hg.mozilla.org/mozilla-central/rev/326bcca7b6b6 https://hg.mozilla.org/mozilla-central/rev/048e2f46f743 https://hg.mozilla.org/mozilla-central/rev/a0d138d05c26 https://hg.mozilla.org/mozilla-central/rev/9ff5f0b2f4e0