661445 - Refactor Livemarks service for future schema changes

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

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

We can separate a Livemark object that caches most of the information on the livemark and handles the datastore internally.

This should both slightly improve performances (more caching), make easier to hack on the service for contributors and make easier to change the datastore when we'll change the schema. The separation is not 100% complete though, since the idl still relies on itemIds.

The patch is large, but we have good tests. Nonetheless it may be better to merge it sooner than later, I have a 90% finished patch in queue.

Comment 1

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

Attachment: patch v1.0

I suspect it's easier to read the service rather than the patch, since now it's splitted into 3 main objects it should be an easy read.

I unbitrotted the fix in bug 303567 on top of this since that bug had an additional livemarks test that is useful to increase test coverage. I also did some manual testing for updates.

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 538413 [details] [diff] [review]
patch v1.0

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

generally this looks great. r=me with these questions answered.

::: toolkit/components/places/nsLivemarkService.js
@@ +383,5 @@
> +    this.folderId = aFolderIdOrCreationInfo;
> +  }
> +  else if (typeof(aFolderIdOrCreationInfo) == "object"){
> +    this.folderId =
> +      PlacesUtils.bookmarks.createFolder(aFolderIdOrCreationInfo.parentId,

how do errors here bubble up? same or different from the status quo?

@@ +412,5 @@
> +   * @throws If the folder is invalid.
> +   */
> +  _setAnno: function LM__setAnno(aName, aValue)
> +  {
> +    if (this.folderId) {

why is this check necessary? when would we ever make this call with an id-less livemark object?

@@ +430,5 @@
> +   * @throws If the folder is invalid or the annotation does not exist.
> +   */
> +  _getAnno: function LM__getAnno(aName)
> +  {
> +    if (this.folderId) {

ditto

@@ +446,5 @@
> +   */
> +  _removeAnno: function LM__removeAnno(aName)
> +  {
> +    if (this.folderId) {
> +      return PlacesUtils.annotations.removeItemAnnotation(this.folderId, aName);

kinda ditto - how would we ever get this far with an invalid folder id?

@@ +515,5 @@
> +    return this._expireTime;
> +  },
> +
> +  set loadStatus(aStatus)
> +  {

there's a whole lot of state management here. why not a single "status" anno, with 3 possible values?

@@ +737,5 @@
> +        }
> +
> +        try {
> +          secMan.checkLoadURIWithPrincipal(feedPrincipal, href, SEC_FLAGS);
> +        }

is there a reason why this isn't done inside the feeds code? it seems a bit risky to have every consumer doing this filtering instead of just filtering feed.items inside the nsIFeed implementation.

Comment 3

[Phil Ringnalda (:philor)]

(In reply to comment #2)
> > +          secMan.checkLoadURIWithPrincipal(feedPrincipal, href, SEC_FLAGS);
> > +        }
> 
> is there a reason why this isn't done inside the feeds code? it seems a bit
> risky to have every consumer doing this filtering instead of just filtering
> feed.items inside the nsIFeed implementation.

Ooh, ooh, I know that one! There are no universally true SEC_FLAGS. In this case, SEC_FLAGS is DISALLOW_INHERIT_PRINCIPAL because we're opening on top of whatever the user has open, like a logged-in banking page, and we shouldn't create a bookmark with the URI "javascript:function stealInfo()..."; in the case of a feeds code consumer who is doing something like sticking the feed URI in a link in content, it would not (necessarily) be DISALLOW_INHERIT_PRINCIPAL.

Comment 4

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

(In reply to comment #2)

> ::: toolkit/components/places/nsLivemarkService.js
> @@ +383,5 @@
> > +    this.folderId = aFolderIdOrCreationInfo;
> > +  }
> > +  else if (typeof(aFolderIdOrCreationInfo) == "object"){
> > +    this.folderId =
> > +      PlacesUtils.bookmarks.createFolder(aFolderIdOrCreationInfo.parentId,
> 
> how do errors here bubble up? same or different from the status quo?

I don't think anything changed from before, the service directly calls into the object and this is synchronouys, I will check though, to be on the safe side.

> @@ +412,5 @@
> > +   * @throws If the folder is invalid.
> > +   */
> > +  _setAnno: function LM__setAnno(aName, aValue)
> > +  {
> > +    if (this.folderId) {
> 
> why is this check necessary? when would we ever make this call with an
> id-less livemark object?

So, this may happen after a call to Livemark.terminate(), that happens when a livemark is removed.
Practically, on item remove, I terminate() the Livemark object (this sets folderId = null) and then remove the Livemark from the cache. If the livemark was loading it may try to set/get/remove annotations (status, expiration and such), but since it has gone that would throw. So I use folderId to track whether the object is bound to an existing bookmark.
Maybe I could try/catch and put a comment in the catch to clarify (provided I can check this.folderId in the catch condition). Or use a different tracking property like _alive or _valid.

> @@ +515,5 @@
> > +  set loadStatus(aStatus)
> > +  {
> 
> there's a whole lot of state management here. why not a single "status"
> anno, with 3 possible values?

It's a good idea and feasible, but I'd not want to inflate this patch even more, so I suggest a follow-up (it forces to change views code and some util)

> @@ +737,5 @@
> > +        try {
> > +          secMan.checkLoadURIWithPrincipal(feedPrincipal, href, SEC_FLAGS);
> > +        }
> 
> is there a reason why this isn't done inside the feeds code? it seems a bit
> risky to have every consumer doing this filtering instead of just filtering
> feed.items inside the nsIFeed implementation.

What philor said.  If we want to touch this, I'd rather filet a follow-up, I tried to limit myself to internal refactoring without changing compatibility with the external world.  But looks like that was done by design ans expected.

Comment 5

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

Attachment: patch v1.1

Verified error bubbling, introduced a Livemark.alive property to clarify the folderId check, filed Bug 664269 to unify the status annotations.

Comment 6

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

http://hg.mozilla.org/projects/places/rev/2aeae6b1bc56

Comment 7

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

http://hg.mozilla.org/mozilla-central/rev/2aeae6b1bc56