Closed Bug 625295 Opened 9 years ago Closed 5 years ago

Sync bookmarks down according to sortindex so that folders sync first

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: philikon, Assigned: eoger, Mentored)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(2 files, 1 obsolete file)

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.
(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!
(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 :)
This is tiny. I can't believe we haven't done it yet.
Whiteboard: [good first bug][mentor=rnewman][lang=js]
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 !
(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.
Attached patch sort newitemsSplinter Review
GET is requesting newitems to which i applied the sort.
Attachment #744757 - Flags: review?(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.
Attachment #744757 - Flags: review?(rnewman)
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
Think about exposing and overriding a SyncEngine.defaultSortIndex property.
I would liker to work on this bug.
I would like to work on this bug.
I would like to work on this bug.
(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've got the build running.
(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.
Good. Make sure all of Sync's xpcshell-tests pass, then read comment 1 through comment 9, and start writing code!
Mentor: rnewman
Whiteboard: [good first bug][mentor=rnewman][lang=js] → [good first bug][lang=js]
Hi Richard
Can you please assign this bug to me?
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.
Attached patch 625295.patch (obsolete) — Splinter Review
Here's another attempt at it using all that's been discussed.
Attachment #8513713 - Flags: review?(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.
Attachment #8513713 - Flags: review?(rnewman) → feedback+
Attached patch 625295.patchSplinter Review
Okay here it is corrected.
Attachment #8513713 - Attachment is obsolete: true
Attachment #8522404 - Flags: review?(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?
Attachment #8522404 - Flags: review?(rnewman) → review+
Great, thanks for the help also!
https://hg.mozilla.org/mozilla-central/rev/211cc88708bd
Assignee: nobody → edouard.oger
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.