996657 - Turn TopSitesCursorWrapper into a multi-cursor wrapper

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Lucas Rocha (:lucasr)]

This is a patch series incrementally refactors TopSitesCursorWrapper to properly support multiple wrapped cursors. For now, it wraps top and pinned sites. It will also wrap suggested sites in the future.

The end result is an almost full rewrite of TopSitesCursorWrapper. But I thought it would still be good to post the incremental patches as self-contained steps i.e. all tests pass in each step. I'm also posting the end result in case it makes these patches easier to review somehow.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Simplify top sites cursor by introducing row types (r=wesj)

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Change TopSitesCursorWrapper to be a Cursor (r=wesj)

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Store pinned sites cursor and fetch values from it (r=wesj)

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: Streamline schema for TopSitesCursorWrapper (r=wesj)

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: Factor out code to update cursor positions (r=wesj)

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: Factor out code to check valid cursor positions (r=wesj)

Comment 7

[Lucas Rocha (:lucasr)]

Attachment: Final result

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 8406914 [details] [diff] [review]
Simplify top sites cursor by introducing row types (r=wesj)

Prep work to support multiple row types coming from different cursors in the wrapper. This will also be used to determine the type of row, replacing the current isPinned() API.

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 8406915 [details] [diff] [review]
Change TopSitesCursorWrapper to be a Cursor (r=wesj)

Using CursorWrapper doesn't make much sense as we'll be wrapper more than one cursor. There's no functional change here. The code for null column handling had to change a bit so I added an extra test for it here.

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 8406916 [details] [diff] [review]
Store pinned sites cursor and fetch values from it (r=wesj)

In order to streamline the access to the multiple wrapped cursors, I have to keep the pinned sites cursor around. The data structure for tracking pinned positions is kept (as we will need this optimization) but simplified into a SparseBooleanArray.

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 8406918 [details] [diff] [review]
Streamline schema for TopSitesCursorWrapper (r=wesj)

Here's real change. Streamline access to TopSitesCursorWrapper's columns through a well-defined schema. This is necessary because the contents of the wrapper doesn't match any existing table or view. For instance, the wrapper has a notion of 'row type' that doesn't exist in any specific DB table. Row types are necessary for UI telemetry purposes, among other things.

Other relevant details:

- We create a static mapping between column names and indexes. This increases the memory footprint of the wrapper a bit but shouldn't be a problem because we're using very optimized data structures with a very small number of elements (String array and ArrayMap)

- The wrapper dynamically builts mapping between the wrapper cursors' columns and the wrapper's. This is to improve performance when traversing the cursor. Again, this increases memory footprint a bit but not in a relevant way as the mapping is using SparseIntArrays which are essential two int arrays with a small number of element. Again, this is done for performance.

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 8406919 [details] [diff] [review]
Factor out code to update cursor positions (r=wesj)

No functional change. Besides the obvious benefits of tidying the code a bit, this is a necessary refactoring for an optimization I'll post in a follow-up bug.

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 8406920 [details] [diff] [review]
Factor out code to check valid cursor positions (r=wesj)

No functional change. Reduces code duplication.

Comment 14

[Wesley Johnston (:wesj)]

Comment on attachment 8406914 [details] [diff] [review]
Simplify top sites cursor by introducing row types (r=wesj)

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

Leaving this review while I read more patches...

::: mobile/android/base/db/TopSitesCursorWrapper.java
@@ +27,5 @@
>  
> +    private enum RowType {
> +        UNKNOWN,
> +        TOP,
> +        PINNED

I wonder if we'd be better to be generic here, and instead just have "cursor" types and "array" types. I need to look through the rest of these patches to see if something like that could work....

Comment 15

[Wesley Johnston (:wesj)]

Comment on attachment 8406915 [details] [diff] [review]
Change TopSitesCursorWrapper to be a Cursor (r=wesj)

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

Seems good. I'm hoping this will be useful for integrating Hub page queries into topsites as well...

::: mobile/android/base/db/TopSitesCursorWrapper.java
@@ +161,5 @@
>  
>      @Override
> +    public boolean move(int offset) {
> +        return moveToPosition(currentPosition + offset);
> +    }

Just moving this for fun eh? :)

@@ +238,5 @@
> +    public short getShort(int columnIndex) {
> +        throw new UnsupportedOperationException();
> +    }
> +
> +    public byte[] getBlob(int columnIndex) {

@Override

@@ +242,5 @@
> +    public byte[] getBlob(int columnIndex) {
> +        throw new UnsupportedOperationException();
> +    }
> +
> +    public void copyStringToBuffer(int columnIndex, CharArrayBuffer buffer) {

@Override

@@ +353,5 @@
> +    }
> +
> +    @Override
> +    public void close() {
> +        topCursor.close();

I wonder if its worth nulling our arrays here as well. i.e. do callers expect this to throw away memory? Likewise, should isClosed check if arrays are null? The Doc descriptions for close say:

"Closes the Cursor, releasing all of its resources and making it completely invalid."

Comment 16

[Wesley Johnston (:wesj)]

Comment on attachment 8406918 [details] [diff] [review]
Streamline schema for TopSitesCursorWrapper (r=wesj)

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

This is mostly thought process/comments (and a few small bugs). I think I'd be ok with this, but want to read through the rest and hear your thoughts first.

::: mobile/android/base/db/BrowserContract.java
@@ +416,5 @@
> +        private TopSites() {}
> +
> +        public static final int TYPE_BLANK = 0;
> +        public static final int TYPE_TOP = 1;
> +        public static final int TYPE_PINNED = 2;

I was going to suggest we make this an enum and pull the ordinal values out. This is actually a case where I think that would be fine (since we don't actually persist this information anywhere, it wouldn't matter if the enum changed). But its probably a waste.

Also, I don't really like that we're keeping track of differences between pinned/top/blank in here anyway. I'd like to just move to a situation where this holds cursors and is oblivious to their "type". I think that's where we're going if I read this right :)

::: mobile/android/base/db/TopSitesCursorWrapper.java
@@ +35,5 @@
> +    private static final String[] columnNames;
> +    private static final ArrayMap<String, Integer> columnIndexes;
> +
> +    static {
> +        columnNames = new String[7];

Just initialize this where its declared.

@@ +46,5 @@
> +        columnNames[6] = TopSites.TYPE;
> +
> +        columnIndexes = new ArrayMap<String, Integer>(columnNames.length);
> +        for (int i = 0; i < columnNames.length; i++) {
> +            columnIndexes.put(columnNames[i], i);

You could do this in the declaration as well if you want. i.e.:

ArrayMap<String, Integer> columnIndexes = new ArrayMap<String, Integer>(columnnames.length) {{
  for (int i = 0; i < columnnames.length; i++) {
    columnIndexes.put(columnnames[i], i);
  }
}};

Is it worth holding both of these? We call getColumnIndex(String) a lot, so holding a Map of some sort makes sense. The Array of column names is used in a few places, but you can get it with ArrayMap.values().toArray() if you really want...

@@ +51,5 @@
> +        }
> +    }
> +
> +    // Maps column indexes from the wrapper to
> +    // the cursor's.

Don't wrap this line.

@@ +52,5 @@
> +    }
> +
> +    // Maps column indexes from the wrapper to
> +    // the cursor's.
> +    private SparseIntArray topIndexes;

I was going to complain this should be topIndices, but lo and behold Indexes is actually a plural form of Index that some people use... The things you learn reviewing code.

@@ +162,5 @@
> +    }
> +
> +    private void assertValidRowType() {
> +        if (rowType == RowType.UNKNOWN) {
> +            throw new IllegalStateException("Can't get columns with cursor in invalid position");

The message is confusing to the consumer. Something like "No provided cursor holds data at this position" might be better?

@@ +273,5 @@
>  
>      @Override
>      public long getLong(int columnIndex) {
> +        assertValidRowType();
> +        assertValidColumnIndex(columnIndex);

I really hate these types of methods. Maybe its just that you call them "assert" methods but really they throw. I asked jchen before, but if we're not anticipating them getting hit, maybe we're better with real java asserts? Or maybe I just need to come around...

Also, currentCursor can also be null sometimes (UNKNOWN rows will throw here, but BLANK ones wont...). Do we need to check it as well? I guess getColumnIndexForCurrentRowType will return -1, and we'll fall through below. The paths through here are a bit confusing...

@@ +288,5 @@
>      public int getInt(int columnIndex) {
> +        assertValidRowType();
> +        assertValidColumnIndex(columnIndex);
> +
> +        if (columnNames[columnIndex].equals(TopSites.TYPE)) {

Do you need to override this row in getType()? The more I think about it, the more I don't really want this anyway. I'd rather the cursors themselves knew about these things...

@@ +351,5 @@
>  
>      @Override
>      public boolean isNull(int columnIndex) {
> +        assertValidRowType();
> +        assertValidColumnIndex(columnIndex);

I wonder if you could just move these into getColumnIndexForCurrentRowType and save us some duplicate code...

@@ +356,4 @@
>  
> +        final int index = getColumnIndexForCurrentRowType(columnIndex);
> +        if (index >= 0) {
> +            return false;

I think you need to query the real cursor here.

Comment 17

[Wesley Johnston (:wesj)]

Comment on attachment 8406919 [details] [diff] [review]
Factor out code to update cursor positions (r=wesj)

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

Ahh.. This does make keeping the wrapper unaware of its contents hard, but seems like a good problem to solve.

::: mobile/android/base/db/TopSitesCursorWrapper.java
@@ +270,5 @@
>      public boolean moveToPosition(int position) {
>          currentPosition = position;
>  
> +        updateTopCursorPosition(position);
> +        updatePinnedCursorPosition(position);

Dumb, but you mind flipping these? The Pinned sites cursor takes precedence to the top sites one, so I'd like for it to be done first :)

Comment 18

[Wesley Johnston (:wesj)]

Comment on attachment 8406914 [details] [diff] [review]
Simplify top sites cursor by introducing row types (r=wesj)

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

I don't really want to keep this long term, but maybe that's impossible or really hard. I want to move this towards a wrapper that we can use to also bring in hub results (and potentially Cursors form ContentResolvers from 3rd party apps?).

For that to happen, I think we'd need ways to determine the "rank" of those results, as well as things like "type" (i.e. do we show a star on this one? A pin? A twitter badge? A huge card view? etc. I wonder if we should show "stars" next to the labels of bookmarked but not pinned thumbnails right now for consistency...) Probably a bigger project, but I'd like this to help make it easier... and I think its moving that direction.

::: mobile/android/base/db/TopSitesCursorWrapper.java
@@ +31,5 @@
> +        PINNED
> +    }
> +
> +    // Type of content in the current position
> +    private RowType rowType;

Change this to currentRowType. It makes it clearer to me that this is transient and will change as you change rows.

@@ +173,5 @@
>          } else {
>              super.moveToPosition(p2);
>          }
>  
> +        updateRowType();

This feels fragile. I think we'll have to tiptoe a bit here...

Comment 19

[Wesley Johnston (:wesj)]

Comment on attachment 8406916 [details] [diff] [review]
Store pinned sites cursor and fetch values from it (r=wesj)

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

A couple questions. Also, I want this to look different. I'm not sure which patch you want to do that in, but I'll mark this one :)

::: mobile/android/base/db/TopSitesCursorWrapper.java
@@ +32,5 @@
>      // The cursor for the top sites query
>      private final Cursor topCursor;
>  
> +    // The cursor for the pinned sites query
> +    private Cursor pinnedCursor;

I'd really rather you moved towards an array of cursors here. Doing things this way just feels kinda awful to me...

We talked about an index to identify "cursor 0 is a pinned sites cursor, treat it like this". I'd rather the cursor was just internally smart, but I'll take what I can get.

@@ +38,2 @@
>      // Associates pinned sites and their respective positions
> +    private SparseBooleanArray pinnedPositions;

I really want to move all of this out of here tbh. We should wrap the pinnedCursor and let that cursor tell us a row is empty (i.e. moveToPosition to can fail if there's no pinned data at a position, and can do that independently of whether its at the start or end of the cursor).

@@ +92,5 @@
>  
>      private int getPinnedBefore(int position) {
>          int numFound = 0;
>          for (int i = 0; i < position; i++) {
> +            if (pinnedPositions.get(i)) {

This is where my CursorWrapper idea will fail... i.e. we need to know, "When I move the pinned cursor to position 4, its really at position 2, so numFound = 2;)". We may have to special case some CursorWrapper implementations with extra methods to do this.

@@ +326,5 @@
>  
>      @Override
>      public void registerContentObserver(ContentObserver observer) {
>          topCursor.registerContentObserver(observer);
> +        pinnedCursor.registerContentObserver(observer);

This scares me a bit. What happens if the pinned (or top sites) cursor changes underneith this cursor?

Comment 20

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #15)
> Comment on attachment 8406915 [details] [diff] [review]
> Change TopSitesCursorWrapper to be a Cursor (r=wesj)
> 
> Review of attachment 8406915 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems good. I'm hoping this will be useful for integrating Hub page queries
> into topsites as well...
> 
> ::: mobile/android/base/db/TopSitesCursorWrapper.java
> @@ +161,5 @@
> >  
> >      @Override
> > +    public boolean move(int offset) {
> > +        return moveToPosition(currentPosition + offset);
> > +    }
> 
> Just moving this for fun eh? :)

You caught me :-) Wanted to keep 'move' methods together in the file.

> @@ +238,5 @@
> > +    public short getShort(int columnIndex) {
> > +        throw new UnsupportedOperationException();
> > +    }
> > +
> > +    public byte[] getBlob(int columnIndex) {
> 
> @Override

Fixed.

> @@ +242,5 @@
> > +    public byte[] getBlob(int columnIndex) {
> > +        throw new UnsupportedOperationException();
> > +    }
> > +
> > +    public void copyStringToBuffer(int columnIndex, CharArrayBuffer buffer) {
> 
> @Override

Done.

> @@ +353,5 @@
> > +    }
> > +
> > +    @Override
> > +    public void close() {
> > +        topCursor.close();
> 
> I wonder if its worth nulling our arrays here as well. i.e. do callers
> expect this to throw away memory? Likewise, should isClosed check if arrays
> are null? The Doc descriptions for close say:
> 
> "Closes the Cursor, releasing all of its resources and making it completely
> invalid."

Good point. I think nulling the arrays on close is a good idea.

Comment 21

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #16)
> Comment on attachment 8406918 [details] [diff] [review]
> Streamline schema for TopSitesCursorWrapper (r=wesj)
> 
> Review of attachment 8406918 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is mostly thought process/comments (and a few small bugs). I think I'd
> be ok with this, but want to read through the rest and hear your thoughts
> first.
> 
> ::: mobile/android/base/db/BrowserContract.java
> @@ +416,5 @@
> > +        private TopSites() {}
> > +
> > +        public static final int TYPE_BLANK = 0;
> > +        public static final int TYPE_TOP = 1;
> > +        public static final int TYPE_PINNED = 2;
> 
> I was going to suggest we make this an enum and pull the ordinal values out.
> This is actually a case where I think that would be fine (since we don't
> actually persist this information anywhere, it wouldn't matter if the enum
> changed). But its probably a waste.
> 
> Also, I don't really like that we're keeping track of differences between
> pinned/top/blank in here anyway. I'd like to just move to a situation where
> this holds cursors and is oblivious to their "type". I think that's where
> we're going if I read this right :)

Independently of how generic we want to go here, we'll still need a public API for check the type of content a certain row has -- for telemetry purposes and to do a better job handling different row types in the UI too (as you've seen in the patches for bug 997660. 

> ::: mobile/android/base/db/TopSitesCursorWrapper.java
> @@ +35,5 @@
> > +    private static final String[] columnNames;
> > +    private static final ArrayMap<String, Integer> columnIndexes;
> > +
> > +    static {
> > +        columnNames = new String[7];
> 
> Just initialize this where its declared.

Done.

> @@ +46,5 @@
> > +        columnNames[6] = TopSites.TYPE;
> > +
> > +        columnIndexes = new ArrayMap<String, Integer>(columnNames.length);
> > +        for (int i = 0; i < columnNames.length; i++) {
> > +            columnIndexes.put(columnNames[i], i);
> 
> You could do this in the declaration as well if you want. i.e.:
> 
> ArrayMap<String, Integer> columnIndexes = new ArrayMap<String,
> Integer>(columnnames.length) {{
>   for (int i = 0; i < columnnames.length; i++) {
>     columnIndexes.put(columnnames[i], i);
>   }
> }};

Done.

> Is it worth holding both of these? We call getColumnIndex(String) a lot, so
> holding a Map of some sort makes sense. The Array of column names is used in
> a few places, but you can get it with ArrayMap.values().toArray() if you
> really want...

Right, it makes a few APIs a bit more efficient and it's just a static String array with 7 strings anyway :-) I'd be more concerned if this was created per-instance.

> @@ +51,5 @@
> > +        }
> > +    }
> > +
> > +    // Maps column indexes from the wrapper to
> > +    // the cursor's.
> 
> Don't wrap this line.

Fixed.

> @@ +52,5 @@
> > +    }
> > +
> > +    // Maps column indexes from the wrapper to
> > +    // the cursor's.
> > +    private SparseIntArray topIndexes;
> 
> I was going to complain this should be topIndices, but lo and behold Indexes
> is actually a plural form of Index that some people use... The things you
> learn reviewing code.

hehe :-)

> @@ +162,5 @@
> > +    }
> > +
> > +    private void assertValidRowType() {
> > +        if (rowType == RowType.UNKNOWN) {
> > +            throw new IllegalStateException("Can't get columns with cursor in invalid position");
> 
> The message is confusing to the consumer. Something like "No provided cursor
> holds data at this position" might be better?

Fair enough, done.

> @@ +273,5 @@
> >  
> >      @Override
> >      public long getLong(int columnIndex) {
> > +        assertValidRowType();
> > +        assertValidColumnIndex(columnIndex);
> 
> I really hate these types of methods. Maybe its just that you call them
> "assert" methods but really they throw. I asked jchen before, but if we're
> not anticipating them getting hit, maybe we're better with real java
> asserts? Or maybe I just need to come around...

This is just a mechanism for catching obvious bugs in the users of TopSitesCursorWrapper. I don't expect them to get hit in our releases :-)

> Also, currentCursor can also be null sometimes (UNKNOWN rows will throw
> here, but BLANK ones wont...). Do we need to check it as well? I guess
> getColumnIndexForCurrentRowType will return -1, and we'll fall through
> below. The paths through here are a bit confusing...

BLANK (and the TYPE column, see below) are very special cases. They are owned by the wrapper and don't exist in any of the wrapped cursors. In theory, I could simply create a cursor representing all the blank spots but that would be just for the sake of correctness -- not worth it in my opinion. 

The rowType is what tells us we know where the cursor is. UNKNOWN is pretty much: current wrapper position is out of bounds i.e. I could simply replace assertRowType() with a assertValidPosition() that checks (isBeforeFirst() || isAfterLast()) and it would have the exact same behaviour. Would you prefer something like this? I don't feel strongly about either. 

> @@ +288,5 @@
> >      public int getInt(int columnIndex) {
> > +        assertValidRowType();
> > +        assertValidColumnIndex(columnIndex);
> > +
> > +        if (columnNames[columnIndex].equals(TopSites.TYPE)) {
> 
> Do you need to override this row in getType()? The more I think about it,
> the more I don't really want this anyway. I'd rather the cursors themselves
> knew about these things...

This is a column that is implemented by the wrapper and is not present in any of the wrapped cursors. so, the cursor themselves couldn't really know about these things (unless we want to wrap the wrapped cursor just to add this column to them or something). Given that this is a wrapper-only column, all rows have it.

The main intent behind the TYPE column here is to avoid adding custom APIs directly to the cursor (like we used to do with the isPinned() method) and forcing users of TopSitesCursorWrapper to cast cursors (which kinda sucks from a type-safery perspective).

> @@ +351,5 @@
> >  
> >      @Override
> >      public boolean isNull(int columnIndex) {
> > +        assertValidRowType();
> > +        assertValidColumnIndex(columnIndex);
> 
> I wonder if you could just move these into getColumnIndexForCurrentRowType
> and save us some duplicate code...

Done.

> @@ +356,4 @@
> >  
> > +        final int index = getColumnIndexForCurrentRowType(columnIndex);
> > +        if (index >= 0) {
> > +            return false;
> 
> I think you need to query the real cursor here.

Good catch, fixed.

Comment 22

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #18)
> Comment on attachment 8406914 [details] [diff] [review]
> Simplify top sites cursor by introducing row types (r=wesj)
> 
> Review of attachment 8406914 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't really want to keep this long term, but maybe that's impossible or
> really hard. I want to move this towards a wrapper that we can use to also
> bring in hub results (and potentially Cursors form ContentResolvers from 3rd
> party apps?).
> 
> For that to happen, I think we'd need ways to determine the "rank" of those
> results, as well as things like "type" (i.e. do we show a star on this one?
> A pin? A twitter badge? A huge card view? etc. I wonder if we should show
> "stars" next to the labels of bookmarked but not pinned thumbnails right now
> for consistency...) Probably a bigger project, but I'd like this to help
> make it easier... and I think its moving that direction.

I'll assume we agreed to work on the general idea for the framework for wrapping multiple cursors in a follow-up bug. Filed bug 1001397 to track this.

> ::: mobile/android/base/db/TopSitesCursorWrapper.java
> @@ +31,5 @@
> > +        PINNED
> > +    }
> > +
> > +    // Type of content in the current position
> > +    private RowType rowType;
> 
> Change this to currentRowType. It makes it clearer to me that this is
> transient and will change as you change rows.

Good point, done.

> @@ +173,5 @@
> >          } else {
> >              super.moveToPosition(p2);
> >          }
> >  
> > +        updateRowType();
> 
> This feels fragile. I think we'll have to tiptoe a bit here...

This ended up going away in bug 997777 btw. I could only 'see' this could optimized once I got the whole thing working, this is why I did it in a later patch.

Comment 23

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #19)
> Comment on attachment 8406916 [details] [diff] [review]
> Store pinned sites cursor and fetch values from it (r=wesj)
> 
> Review of attachment 8406916 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A couple questions. Also, I want this to look different. I'm not sure which
> patch you want to do that in, but I'll mark this one :)
> 
> ::: mobile/android/base/db/TopSitesCursorWrapper.java
> @@ +32,5 @@
> >      // The cursor for the top sites query
> >      private final Cursor topCursor;
> >  
> > +    // The cursor for the pinned sites query
> > +    private Cursor pinnedCursor;
> 
> I'd really rather you moved towards an array of cursors here. Doing things
> this way just feels kinda awful to me...
> 
> We talked about an index to identify "cursor 0 is a pinned sites cursor,
> treat it like this". I'd rather the cursor was just internally smart, but
> I'll take what I can get.
> 
> @@ +38,2 @@
> >      // Associates pinned sites and their respective positions
> > +    private SparseBooleanArray pinnedPositions;
> 
> I really want to move all of this out of here tbh. We should wrap the
> pinnedCursor and let that cursor tell us a row is empty (i.e. moveToPosition
> to can fail if there's no pinned data at a position, and can do that
> independently of whether its at the start or end of the cursor).
> 
> @@ +92,5 @@
> >  
> >      private int getPinnedBefore(int position) {
> >          int numFound = 0;
> >          for (int i = 0; i < position; i++) {
> > +            if (pinnedPositions.get(i)) {
> 
> This is where my CursorWrapper idea will fail... i.e. we need to know, "When
> I move the pinned cursor to position 4, its really at position 2, so
> numFound = 2;)". We may have to special case some CursorWrapper
> implementations with extra methods to do this.
> 
> @@ +326,5 @@
> >  
> >      @Override
> >      public void registerContentObserver(ContentObserver observer) {
> >          topCursor.registerContentObserver(observer);
> > +        pinnedCursor.registerContentObserver(observer);
> 
> This scares me a bit. What happens if the pinned (or top sites) cursor
> changes underneith this cursor?

This simply means that if any of the wrapped cursors change, the wrapper will report its content has changed too.

Comment 24

[Lucas Rocha (:lucasr)]

Oops, pressed save too fast.

(In reply to Wesley Johnston (:wesj) from comment #19)
> Comment on attachment 8406916 [details] [diff] [review]
> Store pinned sites cursor and fetch values from it (r=wesj)
> 
> Review of attachment 8406916 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A couple questions. Also, I want this to look different. I'm not sure which
> patch you want to do that in, but I'll mark this one :)
> 
> ::: mobile/android/base/db/TopSitesCursorWrapper.java
> @@ +32,5 @@
> >      // The cursor for the top sites query
> >      private final Cursor topCursor;
> >  
> > +    // The cursor for the pinned sites query
> > +    private Cursor pinnedCursor;
> 
> I'd really rather you moved towards an array of cursors here. Doing things
> this way just feels kinda awful to me...
> 
> We talked about an index to identify "cursor 0 is a pinned sites cursor,
> treat it like this". I'd rather the cursor was just internally smart, but
> I'll take what I can get.

Not sure I follow your 'array of cursors' idea. What exactly is 'awful' here? Could you be more specific?

> @@ +38,2 @@
> >      // Associates pinned sites and their respective positions
> > +    private SparseBooleanArray pinnedPositions;
> 
> I really want to move all of this out of here tbh. We should wrap the
> pinnedCursor and let that cursor tell us a row is empty (i.e. moveToPosition
> to can fail if there's no pinned data at a position, and can do that
> independently of whether its at the start or end of the cursor).

Ok, let's work on the 'framework' version of this in bug 1001397. I think I have a pretty good idea about how we can make this happen. 

> @@ +92,5 @@
> >  
> >      private int getPinnedBefore(int position) {
> >          int numFound = 0;
> >          for (int i = 0; i < position; i++) {
> > +            if (pinnedPositions.get(i)) {
> 
> This is where my CursorWrapper idea will fail... i.e. we need to know, "When
> I move the pinned cursor to position 4, its really at position 2, so
> numFound = 2;)". We may have to special case some CursorWrapper
> implementations with extra methods to do this.

Yeah, this shouldn't be too hard though. Again, let's discuss/work on this in bug 1001397.

Comment 25

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #17)
> Comment on attachment 8406919 [details] [diff] [review]
> Factor out code to update cursor positions (r=wesj)
> 
> Review of attachment 8406919 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ahh.. This does make keeping the wrapper unaware of its contents hard, but
> seems like a good problem to solve.
> 
> ::: mobile/android/base/db/TopSitesCursorWrapper.java
> @@ +270,5 @@
> >      public boolean moveToPosition(int position) {
> >          currentPosition = position;
> >  
> > +        updateTopCursorPosition(position);
> > +        updatePinnedCursorPosition(position);
> 
> Dumb, but you mind flipping these? The Pinned sites cursor takes precedence
> to the top sites one, so I'd like for it to be done first :)

You're right, done.

Comment 26

[Lucas Rocha (:lucasr)]

Wes, I went through all your comments/questions and it seems I covered all of them now (at least the ones that are the in the scope of this bug). What else do you think it's still pending?

Comment 27

[Lucas Rocha (:lucasr)]

Ping?

Comment 28

[Wesley Johnston (:wesj)]

(In reply to Lucas Rocha (:lucasr) from comment #21)
> Right, it makes a few APIs a bit more efficient and it's just a static
> String array with 7 strings anyway :-) I'd be more concerned if this was
> created per-instance.

I can buy that :)

> BLANK (and the TYPE column, see below) are very special cases. They are
> owned by the wrapper and don't exist in any of the wrapped cursors. In
> theory, I could simply create a cursor representing all the blank spots but
> that would be just for the sake of correctness -- not worth it in my
> opinion. 

Heh. I've kinda liked that idea, but we can debate it somewhere else.

> The rowType is what tells us we know where the cursor is. UNKNOWN is pretty

I've come around to row type a bit. I just want to disentangle display and database a little, but again, I'm fine doing it somewhere else.

Comment 29

[Wesley Johnston (:wesj)]

Comment on attachment 8406916 [details] [diff] [review]
Store pinned sites cursor and fetch values from it (r=wesj)

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

It sounds like you've addressed comments and other things are progress in other bugs.

Comment 30

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/dec1a28e9f79
https://hg.mozilla.org/integration/fx-team/rev/b947d39f4762
https://hg.mozilla.org/integration/fx-team/rev/aef9a61f4511
https://hg.mozilla.org/integration/fx-team/rev/7df1fee82e3c
https://hg.mozilla.org/integration/fx-team/rev/2b86bb3275ca
https://hg.mozilla.org/integration/fx-team/rev/dadd324e3c4d

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dec1a28e9f79
https://hg.mozilla.org/mozilla-central/rev/b947d39f4762
https://hg.mozilla.org/mozilla-central/rev/aef9a61f4511
https://hg.mozilla.org/mozilla-central/rev/7df1fee82e3c
https://hg.mozilla.org/mozilla-central/rev/2b86bb3275ca
https://hg.mozilla.org/mozilla-central/rev/dadd324e3c4d

Comment 32

[Marco Castelluccio [:marco]]

This made API Level 19 a requirement to build Firefox.

Comment 33

[Lucas Rocha (:lucasr)]

(In reply to Marco Castelluccio [:marco] from comment #32)
> This made API Level 19 a requirement to build Firefox.

What build error are you seeing when building with < 19?

Comment 34

[Marco Castelluccio [:marco]]

(In reply to Lucas Rocha (:lucasr) from comment #33)
> (In reply to Marco Castelluccio [:marco] from comment #32)
> > This made API Level 19 a requirement to build Firefox.
> 
> What build error are you seeing when building with < 19?

mobile/android/base/db/TopSitesCursorWrapper.java:10: error: cannot find symbol
import android.support.v4.util.ArrayMap;
                              ^
  symbol:   class ArrayMap
  location: package android.support.v4.util
mobile/android/base/db/TopSitesCursorWrapper.java:45: error: cannot find symbol
    private static final ArrayMap<String, Integer> columnIndexes =
                         ^
  symbol:   class ArrayMap
  location: class TopSitesCursorWrapper
mobile/android/base/db/TopSitesCursorWrapper.java:46: error: cannot find symbol
            new ArrayMap<String, Integer>(columnNames.length) {{
                ^
  symbol:   class ArrayMap
  location: class TopSitesCursorWrapper

Comment 35

[Marco Castelluccio [:marco]]

Actually, we need the Android Support Library v19 to build.

Comment 36

[Marco Castelluccio [:marco]]

Relevant bugs: bug 991864, bug 990166, bug 990161.

Comment 37

[Lucas Rocha (:lucasr)]

(In reply to Marco Castelluccio [:marco] from comment #35)
> Actually, we need the Android Support Library v19 to build.

Right, I'm removing the dependency on v19 in bug 1008295. Thanks for the report!