750684 - Reader Mode: Implement reading list in awesomebar

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

Initial idea is to promote the Reading List as a top level tab in awesome screen (besides the current All Sites, Bookmarks, and History). Another option is to show it as a folder in the bookmarks tab in awesome screen.

Comment 1

[Patryk Adamczyk [:patryk] UX]

Attachment: Option A

I prefer this option. The reading list would show (at minimum) a favicon but perhaps a thumbnail might be an option too. The top text string would show the article name, and the bottom text string would show original location of the article.

Comment 2

[Patryk Adamczyk [:patryk] UX]

Attachment: Option B

The folder bookmark list have a special folder with a reading list.

Comment 3

[Lucas Rocha (:lucasr)]

(In reply to Patryk Adamczyk (UX) from comment #1)
> Created attachment 625190 [details]
> Option A
> 
> I prefer this option. The reading list would show (at minimum) a favicon but
> perhaps a thumbnail might be an option too. The top text string would show
> the article name, and the bottom text string would show original location of
> the article.

Unfortunately, we don't have enough horizontal space in awesomebar screen for one more tab. If we really want to have Reading List as a tab, I think we have 3 options:
1. Replace the History tab with Reading List (make History accessible from somewhere else? A menu option?)
2. Use icons instead of text labels in each tab. The UI would become less obvious but might work well.
3. Turn the tabs in awesomebar screen into scrollable tabs (http://developer.android.com/design/building-blocks/tabs.html#scrollable). We lose the global visibility of all available tabs though. Not sure scrollable tabs fit well on the design of awesomebar screen.

Thoughts?

Comment 4

[Patryk Adamczyk [:patryk] UX]

Attachment: Folder PNGs

Comment 5

[Patryk Adamczyk [:patryk] UX]

Attachment: Folder PNGs

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: Add new special bookmarks folder to hold reading list items

Comment 7

[Lucas Rocha (:lucasr)]

Attachment: Change LocalBrowserDB to handle reading list's folder

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: Show Reading List folder in the awesomebar screen

FYI: I'll probably folder those patches together for landing. I split them to make them easier to review.

Comment 9

[:Margaret Leibovic]

Comment on attachment 626838 [details] [diff] [review]
Add new special bookmarks folder to hold reading list items

>diff --git a/mobile/android/base/db/BrowserContract.java.in b/mobile/android/base/db/BrowserContract.java.in

>@@ -840,16 +842,21 @@ public class BrowserProvider extends Con

>+        private void upgradeDatabaseFrom8to9(SQLiteDatabase db) {
>+            createOrUpdateSpecialFolder(db, Bookmarks.READING_LIST_FOLDER_GUID,
>+                R.string.bookmarks_folder_reading_list, 5);

What happens if there's already a bookmark at position 5? I'm not exactly sure what role this position field plays, so it may be fine, but I'd like for rnewman to take a look as well to be sure.

Other than that, patch looks good!

Comment 10

[:Margaret Leibovic]

Comment on attachment 626839 [details] [diff] [review]
Change LocalBrowserDB to handle reading list's folder

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

Looks good. r+ with one comment addressed.

::: mobile/android/base/db/LocalBrowserDB.java
@@ -616,4 @@
>                  return super.getLong(columnIndex);
>  
> -            if (columnIndex == getColumnIndex(Bookmarks._ID))
> -                return Bookmarks.FAKE_DESKTOP_FOLDER_ID;

I think we should still be handling the Bookmarks._ID case here. I agree it's smart to also handle it in getInt(), since we use a mishmash of int and long for ids (sigh), but we still call getLong() on the Bookmarks._ID column. For example, we even do it in LocalBrowserDB:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java#393

Comment 11

[:Margaret Leibovic]

Comment on attachment 626840 [details] [diff] [review]
Show Reading List folder in the awesomebar screen

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

Giving an r- because this would break selecting bookmarks after entering and exiting the reading mode folder.

::: mobile/android/base/AwesomeBarTabs.java
@@ +906,5 @@
>              String folderTitle = mBookmarksAdapter.getFolderTitle(position);
>  
> +            String guid = cursor.getString(cursor.getColumnIndexOrThrow(Bookmarks.GUID));
> +            mInReadingList = guid.equals(Bookmarks.READING_LIST_FOLDER_GUID);
> +

What about if the user moves up out of the reading list folder back to the root? We should set mInReadingList back to false in that case. Perhaps this would be better handled in mBookmarksAdapter's refreshCurrentFolder, since that would handle any movement into a different folder.

Comment 12

[:Margaret Leibovic]

(In reply to Margaret Leibovic [:margaret] from comment #10)

> I think we should still be handling the Bookmarks._ID case here. I agree
> it's smart to also handle it in getInt(), since we use a mishmash of int and
> long for ids (sigh), but we still call getLong() on the Bookmarks._ID
> column. For example, we even do it in LocalBrowserDB:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/
> LocalBrowserDB.java#393

I apologize, I didn't think about this enough. This change would only affect the SpecialFoldersCursorWrapper returned from getBookmarksInFolder, and it doesn't look like we ever call getLong() on the Bookmarks._ID column with that cursor. That being said, this long/int business is messy, and we obviously missed this in the past!

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 626838 [details] [diff] [review]
Add new special bookmarks folder to hold reading list items

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

Don't forget to update:

        private boolean isSpecialFolder(ContentValues values) {

to include the new GUID.

::: mobile/android/base/locales/en-US/sync_strings.dtd
@@ +71,5 @@
>  <!ENTITY bookmarks.folder.toolbar.label 'Bookmarks Toolbar'>
>  <!ENTITY bookmarks.folder.unfiled.label 'Unsorted Bookmarks'>
>  <!ENTITY bookmarks.folder.desktop.label 'Desktop Bookmarks'>
>  <!ENTITY bookmarks.folder.mobile.label 'Mobile Bookmarks'>
> +<!ENTITY bookmarks.folder.readinglist.label 'Reading List'>

I backported this to Sync:

https://github.com/mozilla-services/android-sync/commit/802b0dca5a8d188d0ff8ed1456ad7ff77736420e

Comment 14

[Richard Newman [:rnewman]]

(In reply to Margaret Leibovic [:margaret] from comment #9)

> What happens if there's already a bookmark at position 5? I'm not exactly
> sure what role this position field plays, so it may be fine, but I'd like
> for rnewman to take a look as well to be sure.

These are roots, so the chance of a collision is low. And Sync doesn't upload the places root (ID 0) anyway, so a collision should have no consequences. But good spot!

Comment 15

[Lucas Rocha (:lucasr)]

Attachment: Show Reading List folder in the awesomebar screen

Ian and Madhava, need feedback on what message to show when removing an item from the reading list. For now, I'm using "Page removed from your Reading List".

Comment 16

[Lucas Rocha (:lucasr)]

Attachment: Screenshot (Reading List folder)

Here's how it looks now.

Comment 17

[Lucas Rocha (:lucasr)]

Attachment: Add new special bookmarks folder to hold reading list items

I decided to add a small optimization to the reading list handling by have a predefined id for it. This will make all following patches much simpler. Added tests as well.

Comment 18

[Lucas Rocha (:lucasr)]

Attachment: Change LocalBrowserDB to handle reading list's folder

Simpler now.

Comment 19

[Lucas Rocha (:lucasr)]

Attachment: Show Reading List folder in the awesomebar screen

More complete patch with reader handling on all tabs.

Comment 21

[Richard Newman [:rnewman]]

Comment on attachment 627806 [details] [diff] [review]
Add new special bookmarks folder to hold reading list items

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

::: mobile/android/base/db/BrowserContract.java.in
@@ +116,5 @@
>      }
>  
>      // Combined bookmarks and history
>      public static final class Combined implements CommonColumns, URLColumns, HistoryColumns, ImageColumns  {
>          private Combined() {}

Does this imply that you want to support reader-mode history records?

If so, what happens when you visit the same URL with two different display modes, on one or two devices?

::: mobile/android/base/db/BrowserProvider.java.in
@@ +801,5 @@
>              createCombinedWithImagesView(db);
>          }
>  
>          private void upgradeDatabaseFrom5to6(SQLiteDatabase db) {
> +            recreateCombinedWithImagesView(db);

Not directly related to this patch, but:

Will this (and its use in upgradeDatabaseFrom4to5) actually work? That is, does the view produced by this single unversioned view creation method work against a v5 or v6 database?

Generally I'm not in favor of modifying the behavior of migration steps in a later release; these createFoo functions should themselves be versioned, and when their behavior changes you should create/refactor accordingly.

If you can verify that, say, a v3 database can upgrade all the way to a v9 database, then thumbs up, and just bear this in mind for the future.

Please ensure that QA tests these migration paths by installing very old versions of Fennec.

@@ +862,5 @@
>          }
>  
> +        private void upgradeDatabaseFrom8to9(SQLiteDatabase db) {
> +            createOrUpdateSpecialFolder(db, Bookmarks.READING_LIST_FOLDER_GUID,
> +                R.string.bookmarks_folder_reading_list, 5);

N.B., you're inserting a localized string into the database. Your display code should very likely ignore this, and use the localized string from R.string directly.

Comment 22

[Lucas Rocha (:lucasr)]

(In reply to Richard Newman [:rnewman] from comment #21)
> Comment on attachment 627806 [details] [diff] [review]
> Add new special bookmarks folder to hold reading list items
> 
> Review of attachment 627806 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/db/BrowserContract.java.in
> @@ +116,5 @@
> >      }
> >  
> >      // Combined bookmarks and history
> >      public static final class Combined implements CommonColumns, URLColumns, HistoryColumns, ImageColumns  {
> >          private Combined() {}
> 
> Does this imply that you want to support reader-mode history records?

Not sure I follow. A history record will never be in reader mode without a matching bookmark entry for the reading list item. History entries will always have DISPLAY_NORMAL because they don't have Bookmarks.PARENT set.

> If so, what happens when you visit the same URL with two different display
> modes, on one or two devices?

The plan is to never record visits to the reader page in history (see bug 750716).

> ::: mobile/android/base/db/BrowserProvider.java.in
> @@ +801,5 @@
> >              createCombinedWithImagesView(db);
> >          }
> >  
> >          private void upgradeDatabaseFrom5to6(SQLiteDatabase db) {
> > +            recreateCombinedWithImagesView(db);
> 
> Not directly related to this patch, but:
> 
> Will this (and its use in upgradeDatabaseFrom4to5) actually work? That is,
> does the view produced by this single unversioned view creation method work
> against a v5 or v6 database?
> 
> Generally I'm not in favor of modifying the behavior of migration steps in a
> later release; these createFoo functions should themselves be versioned, and
> when their behavior changes you should create/refactor accordingly.
> 
> If you can verify that, say, a v3 database can upgrade all the way to a v9
> database, then thumbs up, and just bear this in mind for the future.

You're absolutely right. I'll update the patch to not change the previous migration.
 
> Please ensure that QA tests these migration paths by installing very old
> versions of Fennec.
> 
> @@ +862,5 @@
> >          }
> >  
> > +        private void upgradeDatabaseFrom8to9(SQLiteDatabase db) {
> > +            createOrUpdateSpecialFolder(db, Bookmarks.READING_LIST_FOLDER_GUID,
> > +                R.string.bookmarks_folder_reading_list, 5);
> 
> N.B., you're inserting a localized string into the database. Your display
> code should very likely ignore this, and use the localized string from
> R.string directly.

This is done in a later patch and yes, I'm using the string from R.string directly in the UI (just like the other special folders).

Comment 23

[Lucas Rocha (:lucasr)]

Attachment: Add new special bookmarks folder to hold reading list items

Fixed the db migration.

Comment 24

[:Margaret Leibovic]

Comment on attachment 627910 [details] [diff] [review]
Add new special bookmarks folder to hold reading list items

Will this correctly make the view for new profiles? I see that you added Combined.DISPLAY to the outer SELECT in createCombinedWithImagesView, but don't you need the additional logic added in createCombinedWithImagesViewOn9 to get actual values for this column?

I agree with rnewman that it's smart to be careful about not breaking the previous migrations, but maybe what you want to do is change the createCombinedWithImagesView call in onCreate to createCombinedWithImagesViewOn9.

Comment 25

[:Margaret Leibovic]

Comment on attachment 627807 [details] [diff] [review]
Change LocalBrowserDB to handle reading list's folder

>diff --git a/mobile/android/base/db/LocalBrowserDB.java b/mobile/android/base/db/LocalBrowserDB.java
>@@ -339,16 +347,38 @@ public class LocalBrowserDB implements B

>         // Cache result for future queries
>         mDesktopBookmarksExist = (count == 1);
>         return mDesktopBookmarksExist;
>     }
> 
>+    private boolean readingListItemsExist(ContentResolver cr) {

...

>+        // Cache result for future queries
>+        mReadingListItemsExist = (count > 0);

Super nit: It would be nice if this check was the same as the check above in desktopBookmarksExist. I don't have a preference for (count == 1) or (count > 0), but could you change one of them so they match?

Comment 26

[:Margaret Leibovic]

Comment on attachment 627808 [details] [diff] [review]
Show Reading List folder in the awesomebar screen

I'm not sure I follow why you need to deal with the display in the history tab, since in comment 22, you said that history entries should always just have DISPLAY_NORMAL.

Comment 27

[Lucas Rocha (:lucasr)]

(In reply to Margaret Leibovic [:margaret] from comment #24)
> Comment on attachment 627910 [details] [diff] [review]
> Add new special bookmarks folder to hold reading list items
> 
> Will this correctly make the view for new profiles? I see that you added
> Combined.DISPLAY to the outer SELECT in createCombinedWithImagesView, but
> don't you need the additional logic added in createCombinedWithImagesViewOn9
> to get actual values for this column?
> 
> I agree with rnewman that it's smart to be careful about not breaking the
> previous migrations, but maybe what you want to do is change the
> createCombinedWithImagesView call in onCreate to
> createCombinedWithImagesViewOn9.

Oopsie. You're right, fixed.

Comment 28

[Lucas Rocha (:lucasr)]

(In reply to Margaret Leibovic [:margaret] from comment #25)
> Comment on attachment 627807 [details] [diff] [review]
> Change LocalBrowserDB to handle reading list's folder
> 
> >diff --git a/mobile/android/base/db/LocalBrowserDB.java b/mobile/android/base/db/LocalBrowserDB.java
> >@@ -339,16 +347,38 @@ public class LocalBrowserDB implements B
> 
> >         // Cache result for future queries
> >         mDesktopBookmarksExist = (count == 1);
> >         return mDesktopBookmarksExist;
> >     }
> > 
> >+    private boolean readingListItemsExist(ContentResolver cr) {
> 
> ...
> 
> >+        // Cache result for future queries
> >+        mReadingListItemsExist = (count > 0);
> 
> Super nit: It would be nice if this check was the same as the check above in
> desktopBookmarksExist. I don't have a preference for (count == 1) or (count
> > 0), but could you change one of them so they match?

Done.

Comment 29

[Lucas Rocha (:lucasr)]

Attachment: Add new special bookmarks folder to hold reading list items

New patch with fix in onCreate.

Comment 30

[Lucas Rocha (:lucasr)]

(In reply to Margaret Leibovic [:margaret] from comment #26)
> Comment on attachment 627808 [details] [diff] [review]
> Show Reading List folder in the awesomebar screen
> 
> I'm not sure I follow why you need to deal with the display in the history
> tab, since in comment 22, you said that history entries should always just
> have DISPLAY_NORMAL.

You're right, we should not handle reading list items in the history tab. I removed this code.

Comment 31

[Lucas Rocha (:lucasr)]

Attachment: Show Reading List folder in the awesomebar screen

Comment 32

[:Margaret Leibovic]

Comment on attachment 629278 [details] [diff] [review]
Add new special bookmarks folder to hold reading list items

>diff --git a/mobile/android/base/db/BrowserProvider.java.in b/mobile/android/base/db/BrowserProvider.java.in

>@@ -393,16 +394,17 @@ public class BrowserProvider extends Con
>                                  // We need to return an _id column because CursorAdapter requires it for its
>                                  // default implementation for the getItemId() method. However, since
>                                  // we're not using this feature in the parts of the UI using this view,
>                                  // we can just use 0 for all rows.
>                                  "0 AS " + Combined._ID + ", " +
>                                  Combined.URL + ", " +
>                                  Combined.TITLE + ", " +
>                                  Combined.VISITS + ", " +
>+                                 Combined.DISPLAY + ", " +

Let's get rid of this change in createCombinedWithImagesView. This method is only called on older database upgrades now, and we should just leave those alone.

>+            db.execSQL("CREATE VIEW IF NOT EXISTS " + VIEW_COMBINED_WITH_IMAGES + " AS" +
>+                    " SELECT " + Combined.BOOKMARK_ID + ", " +
>+                                 Combined.HISTORY_ID + ", " +
>+                                 // We need to return an _id column because CursorAdapter requires it for its
>+                                 // default implementation for the getItemId() method. However, since
>+                                 // we're not using this feature in the parts of the UI using this view,
>+                                 // we can just use 0 for all rows.
>+                                 "0 AS " + Combined._ID + ", " +
>+                                 Combined.URL + ", " +
>+                                 Combined.TITLE + ", " +
>+                                 Combined.VISITS + ", " +
>+                                 Combined.DISPLAY + ", " +
>+                                 Combined.DATE_LAST_VISITED + ", " +
>+                                 qualifyColumn(TABLE_IMAGES, Images.FAVICON) + " AS " + Combined.FAVICON + ", " +
>+                                 qualifyColumn(TABLE_IMAGES, Images.THUMBNAIL) + " AS " + Combined.THUMBNAIL +
>+                    " FROM (" +
>+                        // Bookmarks without history.
>+                        " SELECT " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks._ID) + " AS " + Combined.BOOKMARK_ID + ", " +
>+                                     qualifyColumn(TABLE_BOOKMARKS, Bookmarks.URL) + " AS " + Combined.URL + ", " +
>+                                     qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TITLE) + " AS " + Combined.TITLE + ", " +
>+                                     "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.PARENT) + " WHEN " +
>+                                        Bookmarks.FIXED_READING_LIST_ID + " THEN " + Combined.DISPLAY_READER + " ELSE " +
>+                                        Combined.DISPLAY_NORMAL + " END AS " + Combined.DISPLAY + ", " +
>+                                     "-1 AS " + Combined.HISTORY_ID + ", " +
>+                                     "-1 AS " + Combined.VISITS + ", " +
>+                                     "-1 AS " + Combined.DATE_LAST_VISITED +
>+                        " FROM " + TABLE_BOOKMARKS +
>+                        " WHERE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TYPE)  + " = " + Bookmarks.TYPE_BOOKMARK + " AND " +
>+                                    qualifyColumn(TABLE_BOOKMARKS, Bookmarks.IS_DELETED)  + " = 0 AND " +
>+                                    qualifyColumn(TABLE_BOOKMARKS, Bookmarks.URL) +
>+                                        " NOT IN (SELECT " + History.URL + " FROM " + TABLE_HISTORY + ")" +
>+                        " UNION ALL" +
>+                        // History with and without bookmark.
>+                        " SELECT " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks._ID) + " AS " + Combined.BOOKMARK_ID + ", " +
>+                                     qualifyColumn(TABLE_HISTORY, History.URL) + " AS " + Combined.URL + ", " +
>+                                     // Prioritze bookmark titles over history titles, since the user may have
>+                                     // customized the title for a bookmark.
>+                                     "COALESCE(" + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TITLE) + ", " +
>+                                                   qualifyColumn(TABLE_HISTORY, History.TITLE) +")" + " AS " + Combined.TITLE + ", " +
>+                                     "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.PARENT) + " WHEN " +
>+                                        Bookmarks.FIXED_READING_LIST_ID + " THEN " + Combined.DISPLAY_READER + " ELSE " +
>+                                        Combined.DISPLAY_NORMAL + " END AS " + Combined.DISPLAY + ", " +
>+                                     qualifyColumn(TABLE_HISTORY, History._ID) + " AS " + Combined.HISTORY_ID + ", " +
>+                                     qualifyColumn(TABLE_HISTORY, History.VISITS) + " AS " + Combined.VISITS + ", " +
>+                                     qualifyColumn(TABLE_HISTORY, History.DATE_LAST_VISITED) + " AS " + Combined.DATE_LAST_VISITED +
>+                        " FROM " + TABLE_HISTORY + " LEFT OUTER JOIN " + TABLE_BOOKMARKS +
>+                            " ON " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.URL) + " = " + qualifyColumn(TABLE_HISTORY, History.URL) +
>+                        " WHERE " + qualifyColumn(TABLE_HISTORY, History.URL) + " IS NOT NULL AND " +
>+                                    qualifyColumn(TABLE_HISTORY, History.IS_DELETED)  + " = 0 AND (" +
>+                                        qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TYPE) + " IS NULL OR " +
>+                                        qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TYPE)  + " = " + Bookmarks.TYPE_BOOKMARK + ")" +
>+                    ") LEFT OUTER JOIN " + TABLE_IMAGES +
>+                        " ON " + Combined.URL + " = " + qualifyColumn(TABLE_IMAGES, Images.URL));
>+        }

Did you test executing this statement on a test DB and check to make sure there are sane values for the display column in the resulting view? I remember when first writing this view it was really easy to make small mistakes, and it would be great if we could avoid needing another migration to fix it if there are any errors. It looks good to me, but I think a quick test before landing couldn't hurt.

>diff --git a/mobile/android/base/tests/testBrowserProvider.java.in b/mobile/android/base/tests/testBrowserProvider.java.in
>@@ -1411,9 +1422,65 @@ public class testBrowserProvider extends

>+            ContentUris.parseId(mProvider.insert(mHistoryUri, basicHistory));

>+            ContentUris.parseId(mProvider.insert(mBookmarksUri, basicBookmark));

>+            ContentUris.parseId(mProvider.insert(mHistoryUri, combinedHistory));

>+            ContentUris.parseId(mProvider.insert(mBookmarksUri, combinedBookmark));

Since you're not using the IDs for anything, you can get rid of the ContentUris.parseId() calls around these inserts.

Comment 33

[:Margaret Leibovic]

Comment on attachment 629279 [details] [diff] [review]
Show Reading List folder in the awesomebar screen

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

r+ but get rid of the unnecessary changes.

::: mobile/android/base/AwesomeBarTabs.java
@@ +495,5 @@
>                  historyItem.put(URLColumns.FAVICON, favicon);
>  
>              historyItem.put(Combined.BOOKMARK_ID, bookmarkId);
>              historyItem.put(Combined.HISTORY_ID, historyId);
> +            historyItem.put(Combined.DISPLAY, display);

You can get rid of these changes to keep track of the display with the historyItem, since you're not using it anymore.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +252,5 @@
>                                             Combined.HISTORY_ID,
>                                             Combined.URL,
>                                             Combined.TITLE,
>                                             Combined.FAVICON,
> +                                           Combined.DISPLAY,

You also won't be using this anymore.

Comment 34

[:Margaret Leibovic]

(In reply to Margaret Leibovic [:margaret] from comment #33)

> ::: mobile/android/base/db/LocalBrowserDB.java
> @@ +252,5 @@
> >                                             Combined.HISTORY_ID,
> >                                             Combined.URL,
> >                                             Combined.TITLE,
> >                                             Combined.FAVICON,
> > +                                           Combined.DISPLAY,
> 
> You also won't be using this anymore.

Gr, spliter review doesn't give enough context... this is referring to the change in getRecentHistory.

Comment 35

[Lucas Rocha (:lucasr)]

(In reply to Margaret Leibovic [:margaret] from comment #32)
> Comment on attachment 629278 [details] [diff] [review]
> Add new special bookmarks folder to hold reading list items
> 
> >diff --git a/mobile/android/base/db/BrowserProvider.java.in b/mobile/android/base/db/BrowserProvider.java.in
> 
> >@@ -393,16 +394,17 @@ public class BrowserProvider extends Con
> >                                  // We need to return an _id column because CursorAdapter requires it for its
> >                                  // default implementation for the getItemId() method. However, since
> >                                  // we're not using this feature in the parts of the UI using this view,
> >                                  // we can just use 0 for all rows.
> >                                  "0 AS " + Combined._ID + ", " +
> >                                  Combined.URL + ", " +
> >                                  Combined.TITLE + ", " +
> >                                  Combined.VISITS + ", " +
> >+                                 Combined.DISPLAY + ", " +
> 
> Let's get rid of this change in createCombinedWithImagesView. This method is
> only called on older database upgrades now, and we should just leave those
> alone.

Done.

> >+            db.execSQL("CREATE VIEW IF NOT EXISTS " + VIEW_COMBINED_WITH_IMAGES + " AS" +
> >+                    " SELECT " + Combined.BOOKMARK_ID + ", " +
> >+                                 Combined.HISTORY_ID + ", " +
> >+                                 // We need to return an _id column because CursorAdapter requires it for its
> >+                                 // default implementation for the getItemId() method. However, since
> >+                                 // we're not using this feature in the parts of the UI using this view,
> >+                                 // we can just use 0 for all rows.
> >+                                 "0 AS " + Combined._ID + ", " +
> >+                                 Combined.URL + ", " +
> >+                                 Combined.TITLE + ", " +
> >+                                 Combined.VISITS + ", " +
> >+                                 Combined.DISPLAY + ", " +
> >+                                 Combined.DATE_LAST_VISITED + ", " +
> >+                                 qualifyColumn(TABLE_IMAGES, Images.FAVICON) + " AS " + Combined.FAVICON + ", " +
> >+                                 qualifyColumn(TABLE_IMAGES, Images.THUMBNAIL) + " AS " + Combined.THUMBNAIL +
> >+                    " FROM (" +
> >+                        // Bookmarks without history.
> >+                        " SELECT " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks._ID) + " AS " + Combined.BOOKMARK_ID + ", " +
> >+                                     qualifyColumn(TABLE_BOOKMARKS, Bookmarks.URL) + " AS " + Combined.URL + ", " +
> >+                                     qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TITLE) + " AS " + Combined.TITLE + ", " +
> >+                                     "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.PARENT) + " WHEN " +
> >+                                        Bookmarks.FIXED_READING_LIST_ID + " THEN " + Combined.DISPLAY_READER + " ELSE " +
> >+                                        Combined.DISPLAY_NORMAL + " END AS " + Combined.DISPLAY + ", " +
> >+                                     "-1 AS " + Combined.HISTORY_ID + ", " +
> >+                                     "-1 AS " + Combined.VISITS + ", " +
> >+                                     "-1 AS " + Combined.DATE_LAST_VISITED +
> >+                        " FROM " + TABLE_BOOKMARKS +
> >+                        " WHERE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TYPE)  + " = " + Bookmarks.TYPE_BOOKMARK + " AND " +
> >+                                    qualifyColumn(TABLE_BOOKMARKS, Bookmarks.IS_DELETED)  + " = 0 AND " +
> >+                                    qualifyColumn(TABLE_BOOKMARKS, Bookmarks.URL) +
> >+                                        " NOT IN (SELECT " + History.URL + " FROM " + TABLE_HISTORY + ")" +
> >+                        " UNION ALL" +
> >+                        // History with and without bookmark.
> >+                        " SELECT " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks._ID) + " AS " + Combined.BOOKMARK_ID + ", " +
> >+                                     qualifyColumn(TABLE_HISTORY, History.URL) + " AS " + Combined.URL + ", " +
> >+                                     // Prioritze bookmark titles over history titles, since the user may have
> >+                                     // customized the title for a bookmark.
> >+                                     "COALESCE(" + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TITLE) + ", " +
> >+                                                   qualifyColumn(TABLE_HISTORY, History.TITLE) +")" + " AS " + Combined.TITLE + ", " +
> >+                                     "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.PARENT) + " WHEN " +
> >+                                        Bookmarks.FIXED_READING_LIST_ID + " THEN " + Combined.DISPLAY_READER + " ELSE " +
> >+                                        Combined.DISPLAY_NORMAL + " END AS " + Combined.DISPLAY + ", " +
> >+                                     qualifyColumn(TABLE_HISTORY, History._ID) + " AS " + Combined.HISTORY_ID + ", " +
> >+                                     qualifyColumn(TABLE_HISTORY, History.VISITS) + " AS " + Combined.VISITS + ", " +
> >+                                     qualifyColumn(TABLE_HISTORY, History.DATE_LAST_VISITED) + " AS " + Combined.DATE_LAST_VISITED +
> >+                        " FROM " + TABLE_HISTORY + " LEFT OUTER JOIN " + TABLE_BOOKMARKS +
> >+                            " ON " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.URL) + " = " + qualifyColumn(TABLE_HISTORY, History.URL) +
> >+                        " WHERE " + qualifyColumn(TABLE_HISTORY, History.URL) + " IS NOT NULL AND " +
> >+                                    qualifyColumn(TABLE_HISTORY, History.IS_DELETED)  + " = 0 AND (" +
> >+                                        qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TYPE) + " IS NULL OR " +
> >+                                        qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TYPE)  + " = " + Bookmarks.TYPE_BOOKMARK + ")" +
> >+                    ") LEFT OUTER JOIN " + TABLE_IMAGES +
> >+                        " ON " + Combined.URL + " = " + qualifyColumn(TABLE_IMAGES, Images.URL));
> >+        }
> 
> Did you test executing this statement on a test DB and check to make sure
> there are sane values for the display column in the resulting view? I
> remember when first writing this view it was really easy to make small
> mistakes, and it would be great if we could avoid needing another migration
> to fix it if there are any errors. It looks good to me, but I think a quick
> test before landing couldn't hurt.

This is why I wrote the test :-)
 
> >diff --git a/mobile/android/base/tests/testBrowserProvider.java.in b/mobile/android/base/tests/testBrowserProvider.java.in
> >@@ -1411,9 +1422,65 @@ public class testBrowserProvider extends
> 
> >+            ContentUris.parseId(mProvider.insert(mHistoryUri, basicHistory));
> 
> >+            ContentUris.parseId(mProvider.insert(mBookmarksUri, basicBookmark));
> 
> >+            ContentUris.parseId(mProvider.insert(mHistoryUri, combinedHistory));
> 
> >+            ContentUris.parseId(mProvider.insert(mBookmarksUri, combinedBookmark));
> 
> Since you're not using the IDs for anything, you can get rid of the
> ContentUris.parseId() calls around these inserts.

Nice catch, done.

Comment 36

[Lucas Rocha (:lucasr)]

(In reply to Margaret Leibovic [:margaret] from comment #33)
> Comment on attachment 629279 [details] [diff] [review]
> Show Reading List folder in the awesomebar screen
> 
> Review of attachment 629279 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ but get rid of the unnecessary changes.
> 
> ::: mobile/android/base/AwesomeBarTabs.java
> @@ +495,5 @@
> >                  historyItem.put(URLColumns.FAVICON, favicon);
> >  
> >              historyItem.put(Combined.BOOKMARK_ID, bookmarkId);
> >              historyItem.put(Combined.HISTORY_ID, historyId);
> > +            historyItem.put(Combined.DISPLAY, display);
> 
> You can get rid of these changes to keep track of the display with the
> historyItem, since you're not using it anymore.
>
> ::: mobile/android/base/db/LocalBrowserDB.java
> @@ +252,5 @@
> >                                             Combined.HISTORY_ID,
> >                                             Combined.URL,
> >                                             Combined.TITLE,
> >                                             Combined.FAVICON,
> > +                                           Combined.DISPLAY,
> 
> You also won't be using this anymore.

I'll need these changes in bug 757776 so I moved them to the patches there for the sake of correctness.

Comment 37

[Lucas Rocha (:lucasr)]

Pushed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/b0dd2b70a233
http://hg.mozilla.org/integration/mozilla-inbound/rev/dbb910ec58ab

Comment 38

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b0dd2b70a233
https://hg.mozilla.org/mozilla-central/rev/dbb910ec58ab

Comment 39

[Lucas Rocha (:lucasr)]

This is in central now, closing.

Comment 40

[Andreea Pod]

Reading list was implemented in awesomebar. Closing bug as verified fixed on:
Firefox 19.0b5 build 2 (2013-02-06), Nightly 21.0a1 (2013-02-10), Aurora 20.0a2 (2013-02-10) using HTC Desire Z (Android 2.3.3)