Closed Bug 1293790 Opened 4 years ago Closed 3 years ago

Implement AS topsites view

Categories

(Firefox for Android :: Awesomescreen, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 51
Iteration:
1.3
Tracking Status
firefox51 --- fixed

People

(Reporter: ahunt, Assigned: ahunt)

References

Details

(Whiteboard: [MobileAS])

Attachments

(11 files, 1 obsolete file)

58 bytes, text/x-review-board-request
Grisha
: review+
Details
58 bytes, text/x-review-board-request
Grisha
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
109.48 KB, image/png
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
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).
Assignee: nobody → ahunt
Blocks: 1288106
Whiteboard: [MobileAS s1.2]
Attached image preview screnshot (obsolete) —
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.)
Status: NEW → ASSIGNED
Attachment #8779521 - Flags: review?(gkruglov)
Attachment #8779522 - Flags: review?(gkruglov)
Attachment #8779524 - Flags: review?(s.kaspari)
Attachment #8779523 - Flags: review?(s.kaspari)
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 ?
Attachment #8779521 - Flags: review?(gkruglov)
Attachment #8779522 - Flags: review?(gkruglov)
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) { ... }`
Attachment #8779521 - Flags: review?(gkruglov) → review+
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. :)
Attachment #8779523 - Flags: review?(s.kaspari) → review+
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
Attachment #8779524 - Flags: review?(s.kaspari) → review+
Comment on attachment 8782116 [details]
Bug 1293790 - Pre: extract CardView corner workaround into FilledCardView

https://reviewboard.mozilla.org/r/72342/#review70188
Attachment #8782116 - Flags: review?(s.kaspari) → review+
Comment on attachment 8779521 [details]
Bug 1293790 - Implement getPlainTopSites query

(re-requesting review because the patch has changed significantly)
Attachment #8779521 - Flags: review+ → review?(gkruglov)
Priority: -- → P1
Whiteboard: [MobileAS s1.2] → [MobileAS]
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?
Attachment #8779521 - Flags: review?(gkruglov) → review+
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.
Priority: P1 → P2
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).
Priority: P2 → P1
Attached image topsites_paged.png
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
Attachment #8779516 - Attachment is obsolete: true
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!
Attachment #8779522 - Flags: review?(gkruglov) → review+
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.
Attachment #8784476 - Flags: review?(s.kaspari) → review+
Comment on attachment 8784477 [details]
Bug 1293790 - Increase separation between circles in indicator

https://reviewboard.mozilla.org/r/73920/#review72088
Attachment #8784477 - Flags: review?(s.kaspari) → review+
Comment on attachment 8784478 [details]
Bug 1293790 - Add indicator to topsites pager

https://reviewboard.mozilla.org/r/73922/#review72090
Attachment #8784478 - Flags: review?(s.kaspari) → review+
Blocks: 1299201
Blocks: 1298968
Blocks: 1299224
No longer blocks: 1298968
Iteration: --- → 1.3
Depends on: 1354973
You need to log in before you can comment on or make changes to this bug.