1081254 - Implement update in Bookmarks.jsm

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

We still need to implement this API.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1

Comment 2

[Asaf Romano (gone)]

Comment on attachment 8504561 [details] [diff] [review]
patch v1

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

Please merge this patch with the main implementation patch. I prefer to review the next revisions a single patch.

::: toolkit/components/places/Bookmarks.jsm
@@ +158,5 @@
>          info.index = parent._childCount;
>        }
>  
>        let item = yield insertBookmark(info, parent);
> +      return copyPropsForOutput(info, item);

See my comments in the main bug. I'm not sure this makes sense (same goes for the other methods).

@@ +200,2 @@
>      return Task.spawn(function* () {
> +      // We must ensure the item exists.

s/We must ensure/Ensure/

@@ +207,5 @@
> +      if (info.dateAdded && info.dateAdded.getTime() != item.dateAdded.getTime())
> +        throw new Error("The bookmark dateAdded cannot be changed");
> +
> +      // Remove any property that will stay the same.
> +      removeSameValueProperties(info, item);

It seems removeSameValueProperties expects info.url to be a URL object already at this point, but you validate it afterwards. Or is it validated anyway, and the later check just ensure it's a bookmark? In any event, you should document this two-stages validation a little bit.

@@ +219,5 @@
> +                                                       validIf: b => b.lastModified >= item.dateAdded }
> +                                     });
> +
> +        let parent;
> +        if ("parentGuid" in info) {

if you use getOwnPropertyNames as you do, prefer hasOwenProperty, here and elsewhere.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8504561 [details] [diff] [review]
patch v1

Patch merged into Bug 1068009.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

fixed by Bug 1068009.