Closed Bug 917455 Opened 6 years ago Closed 6 years ago

Update top sites query to return non-bookmarks

Categories

(Firefox for Android :: Awesomescreen, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 27
Tracking Status
firefox26 + fixed
firefox27 --- fixed
fennec 26+ ---

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(4 files, 1 obsolete file)

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.
tracking-fennec: --- → ?
Linking this to bug 885084, since I'm basically going to revert the changes from it in here.
Depends on: 885084
Attached patch patchSplinter Review
I did a backout of the patch in bug 885084, then updated it for where getTopSites is called now.
Attachment #806720 - Flags: review?(wjohnston)
Blocks: 917770
Attachment #806720 - Flags: review?(wjohnston) → review+
Attached patch Patch: Single Cursor (obsolete) — Splinter Review
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.
Attachment #806849 - Flags: review?(lucasr.at.mozilla)
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);
Attachment #806849 - Flags: review?(lucasr.at.mozilla) → review-
(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.
> 
> @@ +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.
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.
Attachment #806904 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 806904 [details] [diff] [review]
Patch: Single Cursor

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

Ship it!
Attachment #806904 - Flags: review?(lucasr.at.mozilla) → review+
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
Attachment #806840 - Flags: review?(wjohnston) → review+
This will break build btw. Sriram's patch will fix it.
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.
Attachment #806849 - Attachment is obsolete: true
(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.
(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.
Setting to track 26, since this is part of the follow-up work to bug 917394.
tracking-fennec: ? → 26+
Blocks: 907988
Attachment #807919 - Flags: review?(sriram) → review+
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)
Attachment #806720 - Flags: approval-mozilla-aurora?
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)
Attachment #806840 - Flags: approval-mozilla-aurora?
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)
Attachment #806904 - Flags: approval-mozilla-aurora?
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)
Attachment #807919 - Flags: approval-mozilla-aurora?
Attachment #806720 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #806840 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #806904 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #807919 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.