Closed Bug 661445 Opened 9 years ago Closed 9 years ago

Refactor Livemarks service for future schema changes

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: mak, Assigned: mak)

References

Details

(Whiteboard: [fixed-in-places])

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 303567
Attached patch patch v1.0 (obsolete) — Splinter Review
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.
Attachment #538413 - Flags: review?(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.
Attachment #538413 - Flags: review?(dietrich) → review+
(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.
(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.
Attached patch patch v1.1Splinter Review
Verified error bubbling, introduced a Livemark.alive property to clarify the folderId check, filed Bug 664269 to unify the status annotations.
Attachment #538413 - Attachment is obsolete: true
http://hg.mozilla.org/projects/places/rev/2aeae6b1bc56
Whiteboard: [fixed-in-places]
http://hg.mozilla.org/mozilla-central/rev/2aeae6b1bc56
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Blocks: 549471
Blocks: 638412
You need to log in before you can comment on or make changes to this bug.