Closed Bug 997782 Opened 8 years ago Closed 8 years ago

Use suggested sites in the top sites panels

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 32

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

Attachments

(4 files, 1 obsolete file)

Which means:
- Not fetching unvisited bookmarks in the top sites query
- Fetching suggested sites for the blank spots in the grid
Blocks: 997780
Comment on attachment 8408403 [details] [diff] [review]
Only include visited pages in the top sites query (r=wesj)

So that, on first run, ew show suggested sites instead of unvisited default bookmarks.
Attachment #8408403 - Flags: review?(wjohnston)
Comment on attachment 8408404 [details] [diff] [review]
Precompute number of pinned sites before current position (r=wesj)

We'll need the pinnedBefore value when computing the new position for the suggested sites cursor too.
Attachment #8408404 - Flags: review?(wjohnston)
Comment on attachment 8408405 [details] [diff] [review]
Wrap suggested sites in TopSitesCursorWrapper (r=wesj)

Still not used in the UI. Added a test to make sure nothing obvious breaks when the suggested sites is null (a valid case if there are no spare spots in the grid for suggested sites, for example).
Attachment #8408405 - Flags: review?(wjohnston)
Comment on attachment 8408406 [details] [diff] [review]
Use suggested sites in the top sites query (r=wesj)

Actually integrates SuggestedSites into BrowserDB. Disabling suggested sites should be as simple as not calling BrowserDB.setSuggestedSites().
Attachment #8408406 - Flags: review?(wjohnston)
Comment on attachment 8408403 [details] [diff] [review]
Only include visited pages in the top sites query (r=wesj)

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

Can you update the comment too? These queries are hard to parse :)
Attachment #8408403 - Flags: review?(wjohnston) → review+
Attachment #8408404 - Flags: review?(wjohnston) → review+
Comment on attachment 8408405 [details] [diff] [review]
Wrap suggested sites in TopSitesCursorWrapper (r=wesj)

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

I'm a little confused about the interaction here, but it doesn't match what I read in comment 1 "Fetching suggested sites for the blank spots in the grid".

::: mobile/android/base/db/TopSitesCursorWrapper.java
@@ +146,5 @@
>  
>      private void updateCount() {
> +        int sum = pinnedCursor.getCount() + topCursor.getCount();
> +        if (suggestedCursor != null) {
> +            sum += suggestedCursor.getCount();

We only show suggested sites in the top 6 if there's nothing else there, right?

@@ +158,4 @@
>      }
>  
>      private void updateRowState() {
> +        if (cursorHasValidPosition(suggestedCursor)) {

This will favor suggestions over all other types right? That doesn't seem right to me and doesn't match comment 1.

@@ +499,5 @@
>          topCursor.registerContentObserver(observer);
>          pinnedCursor.registerContentObserver(observer);
> +
> +        if (suggestedCursor != null) {
> +            suggestedCursor.registerContentObserver(observer);

These still make me nervous. Like I said in the other bug, since these cursors aren't final, they could change and we wouldn't update the observers.

::: mobile/android/tests/browser/junit3/src/tests/TestTopSitesCursorWrapper.java
@@ +154,5 @@
>          assertEquals(13, c.getCount());
>          c.close();
> +
> +        // The sum of suggested and pinned sites is greater than MIN_COUNT
> +        c = createTopSitesCursorWrapper(0, new Integer[] { 0, 1 }, 8);

Same thing as above. Is this what we want?

@@ +161,5 @@
> +
> +        // The sum of top, pinned, and suggested sites is greater than MIN_COUNT
> +        c = createTopSitesCursorWrapper(2, new Integer[] { 0, 1, 2 }, 2);
> +        assertEquals(7, c.getCount());
> +        c.close();

You're only missing one test in here (suggested, history, but nothing pinned). Since thats probably the most common case, seems good to test.
Attachment #8408405 - Flags: review?(wjohnston) → review-
Comment on attachment 8408406 [details] [diff] [review]
Use suggested sites in the top sites query (r=wesj)

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

::: mobile/android/base/db/BrowserDB.java
@@ +171,5 @@
> +        Cursor suggestedSites = null;
> +        if (sSuggestedSites != null) {
> +            final int count = minLimit - pinnedCount - topSites.getCount();
> +            if (count > 0) {
> +                suggestedSites = sSuggestedSites.get(count);

Ahh. here's where you're forcing this list size :)
Attachment #8408406 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #11)
> Comment on attachment 8408406 [details] [diff] [review]
> Use suggested sites in the top sites query (r=wesj)
> 
> Review of attachment 8408406 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/db/BrowserDB.java
> @@ +171,5 @@
> > +        Cursor suggestedSites = null;
> > +        if (sSuggestedSites != null) {
> > +            final int count = minLimit - pinnedCount - topSites.getCount();
> > +            if (count > 0) {
> > +                suggestedSites = sSuggestedSites.get(count);
> 
> Ahh. here's where you're forcing this list size :)

Yep :-) Note that this is not just about forcing the list size. This is also about making sure we only load suggested sites if there are available spots for them.
(In reply to Wesley Johnston (:wesj) from comment #10)
> Comment on attachment 8408405 [details] [diff] [review]
> Wrap suggested sites in TopSitesCursorWrapper (r=wesj)
> 
> Review of attachment 8408405 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm a little confused about the interaction here, but it doesn't match what
> I read in comment 1 "Fetching suggested sites for the blank spots in the
> grid".

As you noticed (in comment #11), we only load suggested suggested sites if we number of pinned and top sites is not equal or greater than the number of spots in the grid. As I said in comment #12, this is also done to avoid doing unnecessary queries for suggested sites. 

> ::: mobile/android/base/db/TopSitesCursorWrapper.java
> @@ +146,5 @@
> >  
> >      private void updateCount() {
> > +        int sum = pinnedCursor.getCount() + topCursor.getCount();
> > +        if (suggestedCursor != null) {
> > +            sum += suggestedCursor.getCount();
> 
> We only show suggested sites in the top 6 if there's nothing else there,
> right?

Not exactly. We show suggested sites for all blank spots in the top 6. In other words, it's not an all or nothing kind of thing (if that's what you meant with 'if there's nothing else there').

> @@ +158,4 @@
> >      }
> >  
> >      private void updateRowState() {
> > +        if (cursorHasValidPosition(suggestedCursor)) {
> 
> This will favor suggestions over all other types right? That doesn't seem
> right to me and doesn't match comment 1.

You're right. I updated this patch (to keep each step correct in their own) but this is gone in bug #997777 anyway (I updated the patch there to use the correct precedence order).

> @@ +499,5 @@
> >          topCursor.registerContentObserver(observer);
> >          pinnedCursor.registerContentObserver(observer);
> > +
> > +        if (suggestedCursor != null) {
> > +            suggestedCursor.registerContentObserver(observer);
> 
> These still make me nervous. Like I said in the other bug, since these
> cursors aren't final, they could change and we wouldn't update the observers.

I guess what you mean here is that we'd need to listen to changed in each wrapped cursor and update the wrapper's state accordingly? You're right, let me fix that.

> :::
> mobile/android/tests/browser/junit3/src/tests/TestTopSitesCursorWrapper.java
> @@ +154,5 @@
> >          assertEquals(13, c.getCount());
> >          c.close();
> > +
> > +        // The sum of suggested and pinned sites is greater than MIN_COUNT
> > +        c = createTopSitesCursorWrapper(0, new Integer[] { 0, 1 }, 8);
> 
> Same thing as above. Is this what we want?

You're right, we don't want suggested sites beyond MIN_COUNT. In practice, this case will never happen because, in the UI, we only query suggested sites with pinned+top sites is smaller than MIN_COUNT. I can either simply remove this test (as it doesn't represent a real case) or I could throw an exception if in TopSitesCursorWrapper if suggested sites are being placed beyond minSize. What do you prefer?

> @@ +161,5 @@
> > +
> > +        // The sum of top, pinned, and suggested sites is greater than MIN_COUNT
> > +        c = createTopSitesCursorWrapper(2, new Integer[] { 0, 1, 2 }, 2);
> > +        assertEquals(7, c.getCount());
> > +        c.close();
> 
> You're only missing one test in here (suggested, history, but nothing
> pinned). Since thats probably the most common case, seems good to test.

Good point, done.
(In reply to Lucas Rocha (:lucasr) from comment #13)
> > @@ +499,5 @@
> > >          topCursor.registerContentObserver(observer);
> > >          pinnedCursor.registerContentObserver(observer);
> > > +
> > > +        if (suggestedCursor != null) {
> > > +            suggestedCursor.registerContentObserver(observer);
> > 
> > These still make me nervous. Like I said in the other bug, since these
> > cursors aren't final, they could change and we wouldn't update the observers.
> 
> I guess what you mean here is that we'd need to listen to changed in each
> wrapped cursor and update the wrapper's state accordingly? You're right, let
> me fix that.

Scratch that. These observers never actually update the cursors themselves. This is just a way the ContentResolver has to tell the Cursor user that the cursor needs to be re-queried. In the past, this was done via managedCursor() and auto-requery() (in the main thread, ugh). Nowadays, this the recommended way is to use CursorLoaders, which is what we use in HomePager. In other words, this code simply means that if any of the wrapped cursors change, the wrapper will report its content has changed too.
Flags: needinfo?(wjohnston)
(In reply to Lucas Rocha (:lucasr) from comment #14)
> (In reply to Lucas Rocha (:lucasr) from comment #13)
> > > @@ +499,5 @@
> > > >          topCursor.registerContentObserver(observer);
> > > >          pinnedCursor.registerContentObserver(observer);
> > > > +
> > > > +        if (suggestedCursor != null) {
> > > > +            suggestedCursor.registerContentObserver(observer);
> > > 
> > > These still make me nervous. Like I said in the other bug, since these
> > > cursors aren't final, they could change and we wouldn't update the observers.
> > 
> > I guess what you mean here is that we'd need to listen to changed in each
> > wrapped cursor and update the wrapper's state accordingly? You're right, let
> > me fix that.
> 
> Scratch that. These observers never actually update the cursors themselves.
> This is just a way the ContentResolver has to tell the Cursor user that the
> cursor needs to be re-queried. In the past, this was done via
> managedCursor() and auto-requery() (in the main thread, ugh). Nowadays, this
> the recommended way is to use CursorLoaders, which is what we use in
> HomePager. In other words, this code simply means that if any of the wrapped
> cursors change, the wrapper will report its content has changed too.

Sorry, when I said "change" I meant the cursor object itself could change. i.e. suggestedSites = someOtherCursor(); At that point, the observers wouldn't be attached to the new cursor.
Flags: needinfo?(wjohnston)
(In reply to Wesley Johnston (:wesj) from comment #15)
> (In reply to Lucas Rocha (:lucasr) from comment #14)
> > (In reply to Lucas Rocha (:lucasr) from comment #13)
> > > > @@ +499,5 @@
> > > > >          topCursor.registerContentObserver(observer);
> > > > >          pinnedCursor.registerContentObserver(observer);
> > > > > +
> > > > > +        if (suggestedCursor != null) {
> > > > > +            suggestedCursor.registerContentObserver(observer);
> > > > 
> > > > These still make me nervous. Like I said in the other bug, since these
> > > > cursors aren't final, they could change and we wouldn't update the observers.
> > > 
> > > I guess what you mean here is that we'd need to listen to changed in each
> > > wrapped cursor and update the wrapper's state accordingly? You're right, let
> > > me fix that.
> > 
> > Scratch that. These observers never actually update the cursors themselves.
> > This is just a way the ContentResolver has to tell the Cursor user that the
> > cursor needs to be re-queried. In the past, this was done via
> > managedCursor() and auto-requery() (in the main thread, ugh). Nowadays, this
> > the recommended way is to use CursorLoaders, which is what we use in
> > HomePager. In other words, this code simply means that if any of the wrapped
> > cursors change, the wrapper will report its content has changed too.
> 
> Sorry, when I said "change" I meant the cursor object itself could change.
> i.e. suggestedSites = someOtherCursor(); At that point, the observers
> wouldn't be attached to the new cursor.

That can't happen because all wrapped cursors are final members. TopSitesCursorWrapper never wraps new cursors once its been created.
Flags: needinfo?(wjohnston)
(In reply to Wesley Johnston (:wesj) from comment #9)
> Comment on attachment 8408403 [details] [diff] [review]
> Only include visited pages in the top sites query (r=wesj)
> 
> Review of attachment 8408403 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you update the comment too? These queries are hard to parse :)

Good point, done.
Blocks: 1007645
Attachment #8408405 - Attachment is obsolete: true
Comment on attachment 8420128 [details] [diff] [review]
Wrap suggested sites in TopSitesCursorWrapper (r=wesj)

This should cover all review comments.
Attachment #8420128 - Flags: review?(mark.finkle)
Flags: needinfo?(wjohnston)
Comment on attachment 8420128 [details] [diff] [review]
Wrap suggested sites in TopSitesCursorWrapper (r=wesj)

Looks like this patch has addressed Wesj's concerns. We can always file a followup if Wesj feels something more is needed.

The TopSitesCursorWrapper is being stretched a bit, but I think it will hold together. I don't want to focus too much on the purity of the class for now since we might be doing some UX changes in response to the SuggestedSites concept that will impact the code anyway.
Attachment #8420128 - Flags: review?(mark.finkle) → review+
Blocks: 997768
We'll need to update any test-cases that check for initial top-sites spots of what used to be bookmarks are now these two suggested sites currently

* Mozilla
* Firefox Marketplace
Status: RESOLVED → VERIFIED
Flags: in-moztrap?(fennec)
Keep in mind that the actual list of suggested sites will be defined in bug 997765.
Ah, thanks.
Flags: in-moztrap?(fennec) → in-moztrap?(ioana.chiorean)
QA Contact: ioana.chiorean
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.