486042 - "Load this bookmark in the sidebar" status not synced

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Fritz]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3
Build Identifier: 

The "Load this bookmark in the sidebar" option for every bookmark in Firefox is not being synced by Weave resulting in it having to be manually set on every client.  This mainly affects applets like Google Talk gadget which encourage users to open them in the sidebar.

Reproducible: Always

Steps to Reproduce:
1. Mark a bookmark as "Load this bookmark in the sidebar" in client one
2. Download from cloud to client two
3. Open bookmark in client
Actual Results:  
The bookmark will open as a full page.

Expected Results:  
The bookmark should open in the sidebar

Comment 1

[bomfog]

Summary typo fix s/snyced/synced

Comment 2

[mishail.mishail]

Attachment: check for "bookmarkProperties/loadInSidebar" annotation

Comment 3

[mishail.mishail]

Comment on attachment 371212 [details] [diff] [review]
check for "bookmarkProperties/loadInSidebar" annotation

Will not sync changes if we'll clear a checkmark

Comment 4

[mishail.mishail]

Attachment: v2 patch

Hope that will work

Comment 5

[mishail.mishail]

Also, the condition in BMT_onItemChanged shall be chaged to:


>    // ignore annotations except for the ones that we sync
>    if (isAnno && (property != "livemark/feedURI" ||
>		   property != "livemark/siteURI" ||
>		   property != "microsummary/generatorURI")
>           &&
>           property != "bookmarkProperties/loadInSidebar")
>	return;

Comment 6

[mishail.mishail]

Attachment: v3 pacth. With changes in BookmarksTracker

(In reply to comment #5)
New version of the patch

Comment 7

[Ed Lee :Mardak]

The patch has been bitrotted by a number of landings from the dependency, but I'll update the patch for those.

Comment 8

[Ed Lee :Mardak]

Comment on attachment 371401 [details] [diff] [review]
v3 pacth. With changes in BookmarksTracker

>@@ -206,16 +206,22 @@ BookmarksStore.prototype = {
>+        this._ans.setItemAnnotation(newId, "bookmarkProperties/loadInSidebar",
>+                                    1, 0,
Places sets the annotation to true instead of 1, so we should follow that even though places only checks for "hasAnnotation."

>@@ -471,16 +488,24 @@ BookmarksStore.prototype = {
>+  _isLoadInSidebar: function BStore__isLoadInSidebar(id) {
>+    try {
>+      return this._ans.itemHasAnnotation(id, "bookmarkProperties/loadInSidebar");
No need to wrap this in a try/catch.

>@@ -714,17 +740,19 @@ BookmarksTracker.prototype = {
>     // ignore annotations except for the ones that we sync
>     if (isAnno && (property != "livemark/feedURI" ||
> 		   property != "livemark/siteURI" ||
>-		   property != "microsummary/generatorURI"))
>+		   property != "microsummary/generatorURI")
>+           &&
>+           property != "bookmarkProperties/loadInSidebar")
Why not follow the existing pattern? We can clean this up with an array check, but we might want to store some non-local array of annotations to track.

Comment 9

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/f96307ab1900

Thanks!