997777 - Optimize moveToPosition() performance in TopSitesCursorWrapper

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Lucas Rocha (:lucasr)]

Right now, it moves the position of all wrapper cursors, even when some of them will not be used for the new position anyway.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Throw exception if pinned site is not within min size (r=wesj)

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Optimize moveToPosition() in TopSitesCursorWrapper (r=wesj)

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 8408360 [details] [diff] [review]
Throw exception if pinned site is not within min size (r=wesj)

The following optimization assumes pinned sites are never beyond minSize. So, let's make sure this is the case first (and catch any existing bugs related to this).

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 8408361 [details] [diff] [review]
Optimize moveToPosition() in TopSitesCursorWrapper (r=wesj)

Avoid moving the position of all wrapped cursors if possible.

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 8408360 [details] [diff] [review]
Throw exception if pinned site is not within min size (r=wesj)

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

This goes directly against my plans to make this multi-cursor-wrapper less aware of the cursors in it. It means the pinned sites cursor will have to be aware of how its being shown.

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 8408361 [details] [diff] [review]
Optimize moveToPosition() in TopSitesCursorWrapper (r=wesj)

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

I really don't mind the min checks here. Its easy to not take them when we do something more generic. I think the throw in the previous patch makes it feel like we are setting an invariant about the pinned sites cursor that I think I don't really love... i.e. pinnedSites shouldn't care where they are, but these wrappers can enforce that we don't show them outside the list.

I do think you should consolidate the logic a bit though into some shared methods (that hopefully will eventually be the only methods when we're done).

::: mobile/android/base/db/TopSitesCursorWrapper.java
@@ +203,3 @@
>          }
>  
> +        if (position >= minSize) {

Removing this bit gets trickier, since we might have 100 (or 9) suggested sites in the cursor but you only want ones that would appear as thumbnails (the 6 on phones) (i.e. this is the place where the cursor needs to know about how its displayed maybe? Or maybe if we really believe in our suggestions, they should show below the top sites grid if we've got them...).

I think we'll need a custom CursorWrapper around the pinned and suggested sites eventually...

@@ +206,5 @@
> +            return false;
> +        }
> +
> +        final int index = position - pinnedBefore - topCursor.getCount();
> +        if (index >= 0 && index < suggestedCursor.getCount()) {

From here on out, this and the topSites cursor are essentially identical. We should just have a single method that takes a cursor, position, and RowType.

Comment 7

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #6)
> Comment on attachment 8408361 [details] [diff] [review]
> Optimize moveToPosition() in TopSitesCursorWrapper (r=wesj)
> 
> Review of attachment 8408361 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I really don't mind the min checks here. Its easy to not take them when we
> do something more generic. I think the throw in the previous patch makes it
> feel like we are setting an invariant about the pinned sites cursor that I
> think I don't really love... i.e. pinnedSites shouldn't care where they are,
> but these wrappers can enforce that we don't show them outside the list.

Ok, I'll drop the patch that enforces pinned site positions to be without min size for now.

> ::: mobile/android/base/db/TopSitesCursorWrapper.java
> @@ +203,3 @@
> >          }
> >  
> > +        if (position >= minSize) {
> 
> Removing this bit gets trickier, since we might have 100 (or 9) suggested
> sites in the cursor but you only want ones that would appear as thumbnails
> (the 6 on phones) (i.e. this is the place where the cursor needs to know
> about how its displayed maybe? Or maybe if we really believe in our
> suggestions, they should show below the top sites grid if we've got them...).
> 
> I think we'll need a custom CursorWrapper around the pinned and suggested
> sites eventually...

Yeah, I expect we'll need some sort of inner wrapper for bug 1001397. 

> @@ +206,5 @@
> > +            return false;
> > +        }
> > +
> > +        final int index = position - pinnedBefore - topCursor.getCount();
> > +        if (index >= 0 && index < suggestedCursor.getCount()) {
> 
> From here on out, this and the topSites cursor are essentially identical. We
> should just have a single method that takes a cursor, position, and RowType.

Good point, done.

Comment 8

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/fa62732307ee

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fa62732307ee