974664 - Remove reading list hacks in sync code

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Android Sync, defect, P5)

Description

[Sola Ogunsakin [:sola]]

Follow-up to bug 959290. Now that there is a dedicated reading list table, remove READING_LIST_FOLDER_GUID from the list of excluded guids in AndroidBrowserBookmarksDataAccessor.

Comment 1

[Sola Ogunsakin [:sola]]

Attachment: bug-974664.patch

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8378754 [details] [diff] [review]
bug-974664.patch

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

Nit: space before "r=" in commit message.

You will definitely break tests with this patch. You should consider making this change as a pull request against android-sync, which saves me having to backport this and run the tests for you!

Other files you need to touch:
  src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java
  test/src/org/mozilla/gecko/background/db/TestBookmarks.java

Other files that can probably have some items removed:
  strings/strings.xml.in
  strings/sync_strings.dtd.in

And the imported files from Fennec:
  src/main/java/org/mozilla/gecko/db/BrowserContract.java

Comment 3

[Sola Ogunsakin [:sola]]

Attachment: bug-974664.patch

ran some tests locally, looks good. I'm currently doing a try run.

Comment 4

[Richard Newman [:rnewman]]

A try run will only test the build process. None of Sync's tests will be run. You'll need an android-sync checkout and to run `mvn integration-test`.

Comment 5

[Sola Ogunsakin [:sola]]

did that as well.. looks good :)

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8390196 [details] [diff] [review]
bug-974664.patch

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

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +1028,5 @@
>  
>          if (guid.equals(Bookmarks.PLACES_FOLDER_GUID))
>              values.put(Bookmarks._ID, Bookmarks.FIXED_ROOT_ID);
> +        else if (guid.equals(Obsolete.READING_LIST_FOLDER_GUID))
> +            values.put(Bookmarks._ID, Obsolete.FIXED_READING_LIST_ID);

Remove this.

@@ +1194,5 @@
>          createOrUpdateAllSpecialFolders(db);
>  
> +        // Create Reading list special folder.
> +        createOrUpdateSpecialFolder(db, Obsolete.READING_LIST_FOLDER_GUID,
> +            R.string.reading_list_title, 5);

Nope!

@@ +1343,5 @@
>      }
>  
>      private void upgradeDatabaseFrom8to9(SQLiteDatabase db) {
> +        createOrUpdateSpecialFolder(db, Obsolete.READING_LIST_FOLDER_GUID,
> +            R.string.reading_list_title, 5);

Do we really need to do this?

::: mobile/android/base/db/LocalBrowserDB.java
@@ -1033,5 @@
>          if (url != null) {
>              // Bookmarks are defined by their URL and Folder.
>              builder.withSelection(Bookmarks.URL + " = ? AND "
> -                                  + Bookmarks.PARENT + " = ? AND "
> -                                  + Bookmarks.PARENT + " != ?",

Wat

Comment 7

[Sola Ogunsakin [:sola]]

(In reply to Richard Newman [:rnewman] from comment #6)
> ::: mobile/android/base/db/BrowserDatabaseHelper.java
> @@ +1028,5 @@
> >  
> >          if (guid.equals(Bookmarks.PLACES_FOLDER_GUID))
> >              values.put(Bookmarks._ID, Bookmarks.FIXED_ROOT_ID);
> > +        else if (guid.equals(Obsolete.READING_LIST_FOLDER_GUID))
> > +            values.put(Bookmarks._ID, Obsolete.FIXED_READING_LIST_ID);
> 
> Remove this.
> 
we need to support creating a special reading list folder for incremental upgrades right?

> @@ +1194,5 @@
> >          createOrUpdateAllSpecialFolders(db);
> >  
> > +        // Create Reading list special folder.
> > +        createOrUpdateSpecialFolder(db, Obsolete.READING_LIST_FOLDER_GUID,
> > +            R.string.reading_list_title, 5);
> 
> Nope!
I was under the impression that the migration needed the reading list special folder. Since I removed this method call from createOrUpdateAllSpecialFolders(), I placed it here. so this is unnecessary?
 
> @@ +1343,5 @@
> >      }
> >  
> >      private void upgradeDatabaseFrom8to9(SQLiteDatabase db) {
> > +        createOrUpdateSpecialFolder(db, Obsolete.READING_LIST_FOLDER_GUID,
> > +            R.string.reading_list_title, 5);
> 
> Do we really need to do this?
> 
Both strings say the same thing. The old string was part the code you asked me to remove from strings.xml.in. 

> ::: mobile/android/base/db/LocalBrowserDB.java
> @@ -1033,5 @@
> >          if (url != null) {
> >              // Bookmarks are defined by their URL and Folder.
> >              builder.withSelection(Bookmarks.URL + " = ? AND "
> > -                                  + Bookmarks.PARENT + " = ? AND "
> > -                                  + Bookmarks.PARENT + " != ?",
> 
> Wat

lol yeah

Comment 8

[Richard Newman [:rnewman]]

(In reply to Sola Ogunsakin [:sola] from comment #7)

> we need to support creating a special reading list folder for incremental
> upgrades right?

...

> I was under the impression that the migration needed the reading list
> special folder. Since I removed this method call from
> createOrUpdateAllSpecialFolders(), I placed it here. so this is unnecessary?

If you're migrating from a pre-reading list version, there will be no reading list items, so no point creating the folder only to delete it again later, right?

If you're migrating from a post-reading list version up to the latest, you don't need to create the folder, because it already exists.

If there's a migration that drops the whole bookmarks table and copies data, the folder should be copied. If there isn't, then no worries.


> > Do we really need to do this?
> > 
> Both strings say the same thing. The old string was part the code you asked
> me to remove from strings.xml.in. 

Not the strings, the creation of the folder.

Comment 9

[Sola Ogunsakin [:sola]]

(In reply to Richard Newman [:rnewman] from comment #8)
> (In reply to Sola Ogunsakin [:sola] from comment #7)
> 
> > we need to support creating a special reading list folder for incremental
> > upgrades right?
> 
> ...
> 
> > I was under the impression that the migration needed the reading list
> > special folder. Since I removed this method call from
> > createOrUpdateAllSpecialFolders(), I placed it here. so this is unnecessary?
> 
> If you're migrating from a pre-reading list version, there will be no
> reading list items, so no point creating the folder only to delete it again
> later, right?
> 
> If you're migrating from a post-reading list version up to the latest, you
> don't need to create the folder, because it already exists.
> 
> If there's a migration that drops the whole bookmarks table and copies data,
> the folder should be copied. If there isn't, then no worries.
> 
> 
> > > Do we really need to do this?
> > > 
> > Both strings say the same thing. The old string was part the code you asked
> > me to remove from strings.xml.in. 
> 
> Not the strings, the creation of the folder.

yeah, its called post reading list creation.

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 8390196 [details] [diff] [review]
bug-974664.patch

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

You haven't addressed my comments.

If the RL folder didn't exist -- i.e., we're migrating from a really old version -- there's no migration work to do.

If the RL folder does exist -- i.e., we're migrating from a newer version -- then we don't need to create it, and we can trim the logic down and remove some stuff, like the title string.

Now is the time to do this work, because the understanding will be lost a year from now.

Comment 11

[Richard Newman [:rnewman]]

I will finish this up.

Comment 12

[Firefox Bug Husbandry Bot]

Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.

Comment 13

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.