Closed
Bug 917455
Opened 11 years ago
Closed 11 years ago
Update top sites query to return non-bookmarks
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect)
Tracking
(firefox26+ fixed, firefox27 fixed, fennec26+)
RESOLVED
FIXED
Firefox 27
People
(Reporter: Margaret, Assigned: Margaret)
References
Details
Attachments
(4 files, 1 obsolete file)
5.47 KB,
patch
|
wesj
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.64 KB,
patch
|
wesj
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
18.52 KB,
patch
|
lucasr
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.43 KB,
patch
|
sriram
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•11 years ago
|
tracking-fennec: --- → ?
Assignee | ||
Comment 1•11 years ago
|
||
Linking this to bug 885084, since I'm basically going to revert the changes from it in here.
Depends on: 885084
Assignee | ||
Comment 2•11 years ago
|
||
I did a backout of the patch in bug 885084, then updated it for where getTopSites is called now.
Attachment #806720 -
Flags: review?(wjohnston)
Updated•11 years ago
|
Attachment #806720 -
Flags: review?(wjohnston) → review+
Assignee | ||
Comment 3•11 years ago
|
||
Comment 4•11 years ago
|
||
Attachment #806840 -
Flags: review?(wjohnston)
Comment 5•11 years ago
|
||
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 6•11 years ago
|
||
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-
Comment 7•11 years ago
|
||
(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•11 years ago
|
||
>
> @@ +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•11 years ago
|
||
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 10•11 years ago
|
||
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 11•11 years ago
|
||
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+
Comment 12•11 years ago
|
||
Comment 13•11 years ago
|
||
This will break build btw. Sriram's patch will fix it.
Comment 14•11 years ago
|
||
Comment 15•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #806849 -
Attachment is obsolete: true
Assignee | ||
Comment 16•11 years ago
|
||
(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•11 years ago
|
||
(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.
Assignee | ||
Comment 18•11 years ago
|
||
Setting to track 26, since this is part of the follow-up work to bug 917394.
tracking-fennec: ? → 26+
Comment 19•11 years ago
|
||
Attachment #807919 -
Flags: review?(sriram)
Updated•11 years ago
|
Attachment #807919 -
Flags: review?(sriram) → review+
Comment 21•11 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Comment 22•11 years ago
|
||
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 23•11 years ago
|
||
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 24•11 years ago
|
||
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 25•11 years ago
|
||
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?
Updated•11 years ago
|
Attachment #806720 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
Attachment #806840 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
Attachment #806904 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
Attachment #807919 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
Comment 26•11 years ago
|
||
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•