917455 - Update top sites query to return non-bookmarks

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[:Margaret Leibovic]

We also need to update the query to be able to return a cursor for the list view that appears below the thumbnails, which should exclude the sites shown in the thumbnails.

Comment 1

[:Margaret Leibovic]

Linking this to bug 885084, since I'm basically going to revert the changes from it in here.

Comment 2

[:Margaret Leibovic]

Attachment: patch

I did a backout of the patch in bug 885084, then updated it for where getTopSites is called now.

Comment 3

[:Margaret Leibovic]

https://hg.mozilla.org/projects/fig/rev/50cc3096058f

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: Update top sites behaviour for about:home's Top Sites panel

Comment 5

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: Single Cursor

Made both the adapter use a single cursor. This works fine without any issues (so far).

Additionally fixed two things that caught my eye.
1. Use context instead of parent.getContext() in a newView().
2. I had fixed a bug on setting a title for context menu for the top bookmarks. That was missing in this. Added that one line too.

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 806849 [details] [diff] [review]
Patch: Single Cursor

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

Looks good overal. Just want the code duplication around number_of_top_sites sorted out first.

::: mobile/android/base/home/TopSitesGridAdapter.java
@@ +52,5 @@
> +
> +    @Override
> +    public int getCount() {
> +        final int count = super.getCount();
> +        return (count > 0) ? MAX_ENTRIES : 0;

What's the rationale behind this code? I don't follow.

::: mobile/android/base/home/TopSitesPage.java
@@ +166,5 @@
> +                    return;
> +                }
> +
> +                // Absolute position for the adapter.
> +                position += (mGridAdapter.getCount() - headerCount);

I'd use MAX_GRID_ENTRIES here, no?

@@ +467,5 @@
>          }
>  
>          @Override
>          public Cursor loadCursor() {
> +            final int gridSize = getContext().getResources().getInteger(R.integer.number_of_top_sites);

Use the shared MAX_GRID_ENTRIES?

@@ +480,3 @@
>          public VisitedAdapter(Context context, Cursor cursor) {
>              super(context, cursor);
> +            MAX_GRID_ENTRIES = context.getResources().getInteger(R.integer.number_of_top_sites);

You're getting the same value in different places. Maybe just move MAX_GRID_ENTRIES to the parent class and use the same constant everywhere?

@@ +484,5 @@
> +
> +        @Override
> +        public int getCount() {
> +            final int count = super.getCount();
> +            return (count > 0) ? count - MAX_GRID_ENTRIES : 0;

Maybe this is cleaner?

return Math.max(0, super.getCount() - MAX_GRID_ENTRIES);

@@ +493,5 @@
> +            // Ideally this should check for the cursor's size.
> +            // However this would be called only when there is a row and it's being long pressed.
> +            // In this case, we are already showing MAX_GRID_ENTRIES in a grid.
> +            position += MAX_GRID_ENTRIES;
> +            return super.getItem(position);

Maybe this is cleaner?

return super.getItem(position + MAX_GRID_ENTRIES);

Comment 7

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #6)
> Comment on attachment 806849 [details] [diff] [review]
> Patch: Single Cursor
> 
> Review of attachment 806849 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good overal. Just want the code duplication around number_of_top_sites
> sorted out first.
> 
> ::: mobile/android/base/home/TopSitesGridAdapter.java
> @@ +52,5 @@
> > +
> > +    @Override
> > +    public int getCount() {
> > +        final int count = super.getCount();
> > +        return (count > 0) ? MAX_ENTRIES : 0;
> 
> What's the rationale behind this code? I don't follow.

If the cursor is null, super would return 0, in which case we return the same. If there is a cursor, then this view should show only MAX_ENTRIES from it. I like your Math.max() approach. I'll use it.

> 
> ::: mobile/android/base/home/TopSitesPage.java
> @@ +166,5 @@
> > +                    return;
> > +                }
> > +
> > +                // Absolute position for the adapter.
> > +                position += (mGridAdapter.getCount() - headerCount);
> 
> I'd use MAX_GRID_ENTRIES here, no?

Ya. This piece will be called only if there is a cursor and some entries are shown. I'll use it.

> 
> @@ +467,5 @@
> >          }
> >  
> >          @Override
> >          public Cursor loadCursor() {
> > +            final int gridSize = getContext().getResources().getInteger(R.integer.number_of_top_sites);
> 
> Use the shared MAX_GRID_ENTRIES?
> 
> @@ +480,3 @@
> >          public VisitedAdapter(Context context, Cursor cursor) {
> >              super(context, cursor);
> > +            MAX_GRID_ENTRIES = context.getResources().getInteger(R.integer.number_of_top_sites);
> 
> You're getting the same value in different places. Maybe just move
> MAX_GRID_ENTRIES to the parent class and use the same constant everywhere?

Sharing is a problem as the TopSitesGridAdapter is in a separate file. Do you want me to move that into TopSitesPage? In that case, yes, we can re-use it. Until then, the parent can't have and share it.

> 
> @@ +484,5 @@
> > +
> > +        @Override
> > +        public int getCount() {
> > +            final int count = super.getCount();
> > +            return (count > 0) ? count - MAX_GRID_ENTRIES : 0;
> 
> Maybe this is cleaner?
> 
> return Math.max(0, super.getCount() - MAX_GRID_ENTRIES);

I'll use this.

> 
> @@ +493,5 @@
> > +            // Ideally this should check for the cursor's size.
> > +            // However this would be called only when there is a row and it's being long pressed.
> > +            // In this case, we are already showing MAX_GRID_ENTRIES in a grid.
> > +            position += MAX_GRID_ENTRIES;
> > +            return super.getItem(position);
> 
> Maybe this is cleaner?
> 
> return super.getItem(position + MAX_GRID_ENTRIES);

Ok.

Comment 8

[Sriram Ramasubramanian [:sriram]]

> 
> @@ +484,5 @@
> > +
> > +        @Override
> > +        public int getCount() {
> > +            final int count = super.getCount();
> > +            return (count > 0) ? count - MAX_GRID_ENTRIES : 0;
> 
> Maybe this is cleaner?
> 
> return Math.max(0, super.getCount() - MAX_GRID_ENTRIES);

This can't be changed. If the cursor is null and there are no-entries, we be returning a negative value.

Comment 9

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: Single Cursor

I tried making the TopSitesLoader a private class. But android is not happy.

java.lang.illegalargumentexception:%20Object%20returned%20from%20onCreateLoader%20must%20not%20be%20a%20non-static%20inner%20member%20class:%20TopSitesListLoader{429dc350%20id=0}

Hence we need to get the value there too.

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 806904 [details] [diff] [review]
Patch: Single Cursor

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

Ship it!

Comment 11

[Wesley Johnston (:wesj)]

Comment on attachment 806840 [details] [diff] [review]
Update top sites behaviour for about:home's Top Sites panel

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

lgtm

Comment 12

[Lucas Rocha (:lucasr)]

Pushed: http://hg.mozilla.org/projects/fig/rev/02bed6a67511

Comment 13

[Lucas Rocha (:lucasr)]

This will break build btw. Sriram's patch will fix it.

Comment 14

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/projects/fig/rev/69550abdf216

Comment 15

[Lucas Rocha (:lucasr)]

FYI: We just need to update tests (bug 917398) to land now. Once we're ready, I'll fold all patches into one before pushing so that the change is atomic in the repo.

Comment 16

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #15)
> FYI: We just need to update tests (bug 917398) to land now. Once we're
> ready, I'll fold all patches into one before pushing so that the change is
> atomic in the repo.

I assumed we would just merge fig back into m-c when it's green again, but we could make a roll-up patch for this bug to uplift to aurora.

Comment 17

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #16)
> (In reply to Lucas Rocha (:lucasr) from comment #15)
> > FYI: We just need to update tests (bug 917398) to land now. Once we're
> > ready, I'll fold all patches into one before pushing so that the change is
> > atomic in the repo.
> 
> I assumed we would just merge fig back into m-c when it's green again, but
> we could make a roll-up patch for this bug to uplift to aurora.

Quick update: as per discussion on IRC, we'll simply merge fig to m-c to keep hg history intact.

Comment 18

[:Margaret Leibovic]

Setting to track 26, since this is part of the follow-up work to bug 917394.

Comment 19

[Lucas Rocha (:lucasr)]

Attachment: Implement missing getInt() in TopSitesCursorWrapper

Comment 20

[Lucas Rocha (:lucasr)]

http://hg.mozilla.org/projects/fig/rev/354c958ba7d4

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/50cc3096058f
https://hg.mozilla.org/mozilla-central/rev/02bed6a67511
https://hg.mozilla.org/mozilla-central/rev/69550abdf216
https://hg.mozilla.org/mozilla-central/rev/354c958ba7d4

Comment 22

[Lucas Rocha (:lucasr)]

Comment on attachment 806720 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 862793 (the new about:home UI)

User impact if declined: This is part of a series of (high priority) UI changes that we'd like to get in for Firefox 26. Here's a summary:
* History and Top Sites are separate pages again
* Thumbnails in the Top Sites page (instead of Bookmarks page)
* When entering Home, Top Sites is always the default page

Testing completed (on m-c, etc.): This patch has been in our Nightly builds for almost a week now. No obvious regressions found. UI tests have been updated accordingly (see bug 917398).

Risk to taking this patch (and alternatives if risky): There is some level of risk involved here as this is not a trivial change. However, we've agreed that these changes are important enough to be worth the risk. We'd like to avoid shipping a rather big UI change in Firefox 26 to then change it all over again in the following release as this would be a too disruptive experience for our users. Given the risk involved and the fact that we're adding/changing strings, it's important to have these patches uplifted as soon as possible.

String or IDL/UUID changes made by this patch: n/a

This is part of new-new-abouthome (see bug 917394)

Comment 23

[Lucas Rocha (:lucasr)]

Comment on attachment 806840 [details] [diff] [review]
Update top sites behaviour for about:home's Top Sites panel

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 862793 (the new about:home UI)

User impact if declined: This is part of a series of (high priority) UI changes that we'd like to get in for Firefox 26. Here's a summary:
* History and Top Sites are separate pages again
* Thumbnails in the Top Sites page (instead of Bookmarks page)
* When entering Home, Top Sites is always the default page

Testing completed (on m-c, etc.): This patch has been in our Nightly builds for almost a week now. No obvious regressions found. UI tests have been updated accordingly (see bug 917398).

Risk to taking this patch (and alternatives if risky): There is some level of risk involved here as this is not a trivial change. However, we've agreed that these changes are important enough to be worth the risk. We'd like to avoid shipping a rather big UI change in Firefox 26 to then change it all over again in the following release as this would be a too disruptive experience for our users. Given the risk involved and the fact that we're adding/changing strings, it's important to have these patches uplifted as soon as possible.

String or IDL/UUID changes made by this patch: n/a

This is part of new-new-abouthome (see bug 917394)

Comment 24

[Lucas Rocha (:lucasr)]

Comment on attachment 806904 [details] [diff] [review]
Patch: Single Cursor

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 862793 (the new about:home UI)

User impact if declined: This is part of a series of (high priority) UI changes that we'd like to get in for Firefox 26. Here's a summary:
* History and Top Sites are separate pages again
* Thumbnails in the Top Sites page (instead of Bookmarks page)
* When entering Home, Top Sites is always the default page

Testing completed (on m-c, etc.): This patch has been in our Nightly builds for almost a week now. No obvious regressions found. UI tests have been updated accordingly (see bug 917398).

Risk to taking this patch (and alternatives if risky): There is some level of risk involved here as this is not a trivial change. However, we've agreed that these changes are important enough to be worth the risk. We'd like to avoid shipping a rather big UI change in Firefox 26 to then change it all over again in the following release as this would be a too disruptive experience for our users. Given the risk involved and the fact that we're adding/changing strings, it's important to have these patches uplifted as soon as possible.

String or IDL/UUID changes made by this patch: n/a

This is part of new-new-abouthome (see bug 917394)

Comment 25

[Lucas Rocha (:lucasr)]

Comment on attachment 807919 [details] [diff] [review]
Implement missing getInt() in TopSitesCursorWrapper

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 862793 (the new about:home UI)

User impact if declined: This is part of a series of (high priority) UI changes that we'd like to get in for Firefox 26. Here's a summary:
* History and Top Sites are separate pages again
* Thumbnails in the Top Sites page (instead of Bookmarks page)
* When entering Home, Top Sites is always the default page

Testing completed (on m-c, etc.): This patch has been in our Nightly builds for almost a week now. No obvious regressions found. UI tests have been updated accordingly (see bug 917398).

Risk to taking this patch (and alternatives if risky): There is some level of risk involved here as this is not a trivial change. However, we've agreed that these changes are important enough to be worth the risk. We'd like to avoid shipping a rather big UI change in Firefox 26 to then change it all over again in the following release as this would be a too disruptive experience for our users. Given the risk involved and the fact that we're adding/changing strings, it's important to have these patches uplifted as soon as possible.

String or IDL/UUID changes made by this patch: n/a

This is part of new-new-abouthome (see bug 917394)

Comment 26

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7951523d9b54
https://hg.mozilla.org/releases/mozilla-aurora/rev/c444238cb90a
https://hg.mozilla.org/releases/mozilla-aurora/rev/d7e3b0bb4448
https://hg.mozilla.org/releases/mozilla-aurora/rev/832d9813c147