625295 - Sync bookmarks down according to sortindex so that folders sync first

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

Bug 610375 gave bookmark folders a pretty high sortindex, but that doesn't actually ensure they're synced down first on desktop where we get all records in whatever order the server sends them to us. Having folders sync first means we hit the reparenting orphans problem a lot less.

We should add a hook to SyncEngine to allow engine implementations to specify whether they want to sync their data down according to sortindex or not and then enable this for the bookmark engine.

Comment 1

[Richard Newman [:rnewman]]

(In reply to comment #0)
> Having folders sync first means we hit the reparenting orphans problem a lot less.

Not to mention that avoiding reparenting saves a whole bunch of annotation setting, querying, and deleting, as well as modifying the bookmark. Yay speed!

Comment 2

[Philipp von Weitershausen [:philikon]]

(In reply to comment #1)
> (In reply to comment #0)
> > Having folders sync first means we hit the reparenting orphans problem a lot less.
> 
> Not to mention that avoiding reparenting saves a whole bunch of annotation
> setting, querying, and deleting, as well as modifying the bookmark. Yay speed!

Right, that's a better description of what I meant with "reparenting orphans problem". Thanks :)

Comment 3

[Richard Newman [:rnewman]]

This is tiny. I can't believe we haven't done it yet.

Comment 4

[Amod [:AMoz]]

I would like to work on this bug.
In file "services/sync/modules/engines/bookmarks.js", I have noted following :
1. << const FOLDER_SORTINDEX = 1000000; >> is a very high value

In file "services/sync/modules/engines.js", I have noted following :
1. Limit has been imposed on the value at << guidColl.limit = this.downloadLimit; >>

Also at http://docs.services.mozilla.com/storage/apis-1.1.html#url-semantics sort techniques have been mentioned like "sort", "index_above", "index_below"

In Description, I am not able to understand stmnt "We should add a hook to SyncEngine". 
Thanks !

Comment 5

[Richard Newman [:rnewman]]

(In reply to Amod [:greatwarrior] from comment #4)

> In Description, I am not able to understand stmnt "We should add a hook to
> SyncEngine". 

Each engine ultimately makes GET requests to the server to retrieve records. You need to provide a mechanism for the bookmarks engine to cause its prototype (SyncEngine)'s methods to include "sort=index" in that request.

You should read the tests and code for Sync, understand how syncing an engine ultimately causes HTTP requests to occur, and decide what you need to change.

Comment 6

[Amod [:AMoz]]

Attachment: sort newitems

GET is requesting newitems to which i applied the sort.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 744757 [details] [diff] [review]
sort newitems

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

::: services/sync/modules/engines.js
@@ +781,5 @@
>  
>      if (!newitems) {
>        newitems = this._itemSource();
>      }
> +    newitems.sort="index";

This will apply to every engine, which is undesirable. The goal of this bug is to conditionally set this flag for the bookmarks engine only, and otherwise continue to use the default sort order.

Comment 8

[Amod [:AMoz]]

There are two options i can see as appropriate :
1. Add  
> engine.sort="index";
after http://goo.gl/TIlQc

2. Add
> if(this.engine == "BookmarksEngine") {
> newitems.sort="index" 
> }
after http://goo.gl/QmNT4

Comment 9

[Richard Newman [:rnewman]]

Think about exposing and overriding a SyncEngine.defaultSortIndex property.

Comment 10

[Gaurav Saxena]

I would liker to work on this bug.

Comment 11

[Gaurav Saxena]

I would like to work on this bug.

Comment 12

[Gaurav Saxena]

I would like to work on this bug.

Comment 13

[Richard Newman [:rnewman]]

(In reply to Gaurav Saxena from comment #12)
> I would like to work on this bug.

Great! Start here:

https://developer.mozilla.org/docs/Developer_Guide/Build_Instructions#Getting_started

When you've got a build running, and you're able to run the test suites, let us know.

Comment 14

[Gaurav Saxena]

I've got the build running.

Comment 15

[Gaurav Saxena]

(In reply to Richard Newman [:rnewman] from comment #13)
> (In reply to Gaurav Saxena from comment #12)
> > I would like to work on this bug.
> 
> Great! Start here:
> 
> https://developer.mozilla.org/docs/Developer_Guide/
> Build_Instructions#Getting_started
> 
> When you've got a build running, and you're able to run the test suites, let
> us know.

I have got the build running.

Comment 16

[Richard Newman [:rnewman]]

Good. Make sure all of Sync's xpcshell-tests pass, then read comment 1 through comment 9, and start writing code!

Comment 17

[Shah]

Hi Richard
Can you please assign this bug to me?

Comment 18

[Richard Newman [:rnewman]]

Shah, just go ahead start working; we tend to only mark bugs as assigned when there's a patch ready to start the review process.

Comment 19

[Edouard Oger [:eoger]]

Attachment: 625295.patch

Here's another attempt at it using all that's been discussed.

Comment 20

[Richard Newman [:rnewman]]

Comment on attachment 8513713 [details] [diff] [review]
625295.patch

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

::: services/sync/modules/engines.js
@@ +701,5 @@
> +SyncEngine.sortingMode = {
> +  newest: "newest",
> +  index:  "index",
> +  none: undefined
> +};

I don't think we even need this. Just use the values directly.

@@ +709,5 @@
>    _recordObj: CryptoWrapper,
>    version: 1,
>  
> +  // What kind of sorting do we want when asking the
> +  // sync server for records ?

// Which sortorder to use when retrieving records for this engine.

Comment 21

[Edouard Oger [:eoger]]

Attachment: 625295.patch

Okay here it is corrected.

Comment 22

[Richard Newman [:rnewman]]

Comment on attachment 8522404 [details] [diff] [review]
625295.patch

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

Other than the naming nit, and the r= (should be r=rnewman), this looks good, so I'll clean this up and land it.

Thanks for the patch!

::: services/sync/modules/engines.js
@@ +702,5 @@
>    _recordObj: CryptoWrapper,
>    version: 1,
>  
> +  // Which sortorder to use when retrieving records for this engine.
> +  defaultSortIndex: undefined,

defaultSortOrder, no?

Comment 23

[Richard Newman [:rnewman]]

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

Comment 24

[Edouard Oger [:eoger]]

Great, thanks for the help also!

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/211cc88708bd