724745 - Bump parent folder modified time when altering a parent-related child attribute

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

Repositioning, adding, moving, removing: bump the modified time of the parent so we know to upload a fresh record with a correct children array.

Comment 1

[Richard Newman [:rnewman]]

Attachment: Untested patch. v1

Comment 2

[Lucas Rocha (:lucasr)]

Comment on attachment 594946 [details] [diff] [review]
Untested patch. v1

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

Shouldn't this code be in the ContentProvider? If you want to avoid running this code when accessing the ContentProvider from sync, you can simply check of the query arg.

Comment 3

[Richard Newman [:rnewman]]

(In reply to Lucas Rocha (:lucasr) from comment #2)

> Shouldn't this code be in the ContentProvider? If you want to avoid running
> this code when accessing the ContentProvider from sync, you can simply check
> of the query arg.

I see your point. I considered this, and decided against:

* It would be really hard for the CP to decide which records to update. That is, for an arbitrary insert() or update() call -- essentially arbitrary SQL -- which records are parents and need to be touched, and which queries are modifying values in such a way that the parent should be touched.

* … partly because it's arguable whether this is storage-level or app-level logic. The CP doesn't have the context.

* Whether to apply this logic isn't a binary "are we Sync?" decision. Sync will sometimes want to bump parent times, perhaps in bulk, and Fennec won't always want to (e.g., if it's touching multiple records at once).

From my perspective, there will be one or two places that want to issue an update to bump a timestamp in certain situations (adding a bookmark to a folder, for example). That probably doesn't warrant the complexity of adding to the CP.

Do you have another viewpoint?

Comment 4

[Richard Newman [:rnewman]]

From IRC:

09:36 < lucasr> rnewman, hey! about bug 724745, I'm ok with it as long as it works fine for sync
09:36 < rnewman> I'm sorry to be adding a small maintenance burden to app-level code
09:36 < lucasr> I only questioned the cp-level vs app-level because I thought sync would want a similar behaviour as well
09:37 < lucasr> no problem at all
09:37 < rnewman> okee, will test today and upload a final patch for review
09:37 < rnewman> thanks for the quick feedback!

Comment 5

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v2

This patch:

* Introduces a CP URI for dealing with parents (right now only UPDATE).
* Adds an updateBookmarkParents method to do so.
* While we're here, make the CP setting modified time conditional on the input values not having one.

* Adds logging hooks to LocalBrowserDB.
* Adds parent bumping logic to the add and remove methods, including appropriate reuse of computed values.

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 596155 [details] [diff] [review]
Proposed patch. v2

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

Looks good (with nits fixed).

::: mobile/android/base/db/BrowserProvider.java.in
@@ +1018,5 @@
> +        String where = Bookmarks._ID + " IN (" +
> +                       " SELECT DISTINCT " + Bookmarks.PARENT +
> +                       " FROM " + TABLE_BOOKMARKS +
> +                       " WHERE " + selection + " )";
> +        return getWritableDatabase(uri).update(TABLE_BOOKMARKS, values, where, selectionArgs);

Just for the sake of consistency with the rest of the code here, assign the database to a variable then call update(...) on it. It makes code more readable.

final SQLiteDatabase db = getWritableDatabase(uri);
db.update(...);

@@ +1040,5 @@
>                  selection, selectionArgs, null, null, null);
>  
>          try {
> +            if (!values.containsKey(Bookmarks.DATE_MODIFIED))
> +                values.put(Bookmarks.DATE_MODIFIED, System.currentTimeMillis());

Looks like a separate fix? Should go on a separate patch if so.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +67,5 @@
>      public static final int TRUNCATE_N_OLDEST = 5;
>  
> +    // Calculate these once, at initialization. isLoggable is too expensive to
> +    // have in-line in each log call.
> +    private static final String LOGTAG = "GeckoBrowserDB";

GeckoLocalBrowserDB

@@ +382,5 @@
> +        final Uri updateURI =
> +            Bookmarks.CONTENT_URI.buildUpon()
> +                                 .appendPath("parents")
> +                                 .appendQueryParameter(BrowserContract.PARAM_PROFILE, mProfile)
> +                                 .build();

This URI should probably be set once in Bookmarks and used here. So you could do something as simple as appendProfile(Bookmarks.PARENTS_CONTENT_URI) here.

@@ +443,3 @@
>      }
>  
>      public void updateBookmark(ContentResolver cr, String oldUri, String uri, String title, String keyword) {

No parent bump when updating bookmarks?

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 596155 [details] [diff] [review]
Proposed patch. v2

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

Looks good (with nits fixed).

::: mobile/android/base/db/BrowserProvider.java.in
@@ +1018,5 @@
> +        String where = Bookmarks._ID + " IN (" +
> +                       " SELECT DISTINCT " + Bookmarks.PARENT +
> +                       " FROM " + TABLE_BOOKMARKS +
> +                       " WHERE " + selection + " )";
> +        return getWritableDatabase(uri).update(TABLE_BOOKMARKS, values, where, selectionArgs);

Just for the sake of consistency with the rest of the code here, assign the database to a variable then call update(...) on it. It makes code more readable.

final SQLiteDatabase db = getWritableDatabase(uri);
db.update(...);

@@ +1040,5 @@
>                  selection, selectionArgs, null, null, null);
>  
>          try {
> +            if (!values.containsKey(Bookmarks.DATE_MODIFIED))
> +                values.put(Bookmarks.DATE_MODIFIED, System.currentTimeMillis());

Looks like a separate fix? Should go on a separate patch if so.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +67,5 @@
>      public static final int TRUNCATE_N_OLDEST = 5;
>  
> +    // Calculate these once, at initialization. isLoggable is too expensive to
> +    // have in-line in each log call.
> +    private static final String LOGTAG = "GeckoBrowserDB";

GeckoLocalBrowserDB

@@ +382,5 @@
> +        final Uri updateURI =
> +            Bookmarks.CONTENT_URI.buildUpon()
> +                                 .appendPath("parents")
> +                                 .appendQueryParameter(BrowserContract.PARAM_PROFILE, mProfile)
> +                                 .build();

This URI should probably be set once in Bookmarks and used here. So you could do something as simple as appendProfile(Bookmarks.PARENTS_CONTENT_URI) here.

@@ +443,3 @@
>      }
>  
>      public void updateBookmark(ContentResolver cr, String oldUri, String uri, String title, String keyword) {

No parent bump when updating bookmarks?

Comment 8

[Richard Newman [:rnewman]]

(In reply to Lucas Rocha (:lucasr) from comment #7)

> Just for the sake of consistency with the rest of the code here, assign the
> database to a variable then call update(...) on it. It makes code more
> readable.

Heh, I had it that way and inlined it because I'm used to people telling me off for single-use variables :D

> Looks like a separate fix? Should go on a separate patch if so.

Will do.

> >      public void updateBookmark(ContentResolver cr, String oldUri, String uri, String title, String keyword) {
> 
> No parent bump when updating bookmarks?

Nope. None of those updates are relevant to the parent -- they don't change the order or elements of the children, or affect the parent's title or other fields.

If updateBookmark were able to change the position of a bookmark (rather than allowing Sync to do it directly behind the scenes) then we'd add bumping here, too.

Comment 9

[Richard Newman [:rnewman]]

Finally:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b608183760fa

Marking as blocking Bug 727309 because otherwise it'll get messy.

Comment 10

[Marco Bonardo [:mak]]

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