997782 - Use suggested sites in the top sites panels

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Lucas Rocha (:lucasr)]

Which means:
- Not fetching unvisited bookmarks in the top sites query
- Fetching suggested sites for the blank spots in the grid

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Only include visited pages in the top sites query (r=wesj)

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Precompute number of pinned sites before current position (r=wesj)

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Wrap suggested sites in TopSitesCursorWrapper (r=wesj)

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: Use suggested sites in the top sites query (r=wesj)

Comment 5

[Lucas Rocha (:lucasr)]

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.

Comment 6

[Lucas Rocha (:lucasr)]

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.

Comment 7

[Lucas Rocha (:lucasr)]

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).

Comment 8

[Lucas Rocha (:lucasr)]

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().

Comment 9

[Wesley Johnston (:wesj)]

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 :)

Comment 10

[Wesley Johnston (:wesj)]

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.

Comment 11

[Wesley Johnston (:wesj)]

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 :)

Comment 12

[Lucas Rocha (:lucasr)]

(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.

Comment 13

[Lucas Rocha (:lucasr)]

(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.

Comment 14

[Lucas Rocha (:lucasr)]

(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.

Comment 15

[Wesley Johnston (:wesj)]

(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.

Comment 16

[Lucas Rocha (:lucasr)]

(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.

Comment 17

[Lucas Rocha (:lucasr)]

(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.

Comment 18

[Lucas Rocha (:lucasr)]

Attachment: Wrap suggested sites in TopSitesCursorWrapper (r=wesj)

Comment 19

[Lucas Rocha (:lucasr)]

Comment on attachment 8420128 [details] [diff] [review]
Wrap suggested sites in TopSitesCursorWrapper (r=wesj)

This should cover all review comments.

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 21

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/e6bfd8ddbbb9
https://hg.mozilla.org/integration/fx-team/rev/3624b8b95ced
https://hg.mozilla.org/integration/fx-team/rev/937dcbe9cf3d
https://hg.mozilla.org/integration/fx-team/rev/20ea289627c9

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e6bfd8ddbbb9
https://hg.mozilla.org/mozilla-central/rev/3624b8b95ced
https://hg.mozilla.org/mozilla-central/rev/937dcbe9cf3d
https://hg.mozilla.org/mozilla-central/rev/20ea289627c9

Comment 23

[Aaron Train [:aaronmt]]

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

Comment 24

[Lucas Rocha (:lucasr)]

Keep in mind that the actual list of suggested sites will be defined in bug 997765.

Comment 25

[Aaron Train [:aaronmt]]

Ah, thanks.