376919 - hidden pref to disable microsummary functionality

Status: RESOLVED FIXED

(Firefox Graveyard :: Microsummaries, enhancement)

Description

[Myk Melez [:myk] [@mykmelez]]

For folks who don't want the microsummary service to check for microsummaries when they access bookmark properties, we should have a hidden pref for disabling microsummary functionality.

Comment 1

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch

Adds the hidden pref browser.microsummary.enabled and modifies getMicrosummaries() to return an empty set if the pref is false. This allows users to switch the state of the pref without it being destructive to existing microsummaries and bookmarks (current title will be retained as will the mapping to a microsummary even after an edit of the bookmark's properties).

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 261913 [details] [diff] [review]
Patch

> Adds the hidden pref browser.microsummary.enabled and modifies
> getMicrosummaries() to return an empty set if the pref is false. This allows
> users to switch the state of the pref without it being destructive to existing
> microsummaries and bookmarks (current title will be retained as will the
> mapping to a microsummary even after an edit of the bookmark's properties).

Hmm, I suspect that users who disable microsummaries will expect their live titles to change back to regular page titles rather than just freezing in time, since a frozen live title will quickly grow stale and misleading.

But making this non-destructive on RDF-based bookmarks is hard, since there's no property that would allow us to store the fact that the bookmark used to be a live title bookmark and should become one again if microsummaries are reenabled.

So the current approach is good for now, but we should file a followup bug to switch to the static page title when microsummaries are disabled (and back to the live title when they get reenabled) when we switch the trunk over to Places-based bookmarks, where it'll be easier to do this.


>-
>-    this._cacheLocalGenerators();

In general, the patch looks great, but this hunk doesn't make sense.  Is this perhaps a stray change?

r=myk without this hunk, or let me know what this is needed for and re-request review.

Comment 3

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Comment on attachment 261913 [details] [diff] [review]
Patch

(In reply to comment #2)
> >-
> >-    this._cacheLocalGenerators();
> 
> In general, the patch looks great, but this hunk doesn't make sense.  Is this
> perhaps a stray change?
> 
> r=myk without this hunk, or let me know what this is needed for and re-request
> review.
> 

I split _init() into _init() and _initTimer() while keeping the timer code the same. That hunk is just moving _cacheLocalGenerators() back into _init() since it was previously under the timer code.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 261913 [details] [diff] [review]
Patch

> I split _init() into _init() and _initTimer() while keeping the timer code the
> same. That hunk is just moving _cacheLocalGenerators() back into _init() since
> it was previously under the timer code.

Oh, right, duh, I can't believe I missed that.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Patch checked in to the trunk.  Thanks Ryan!

Checking in nsMicrosummaryService.js;
/cvsroot/mozilla/browser/components/microsummaries/src/nsMicrosummaryService.js,v  <--  nsMicrosummaryService.js
new revision: 1.60; previous revision: 1.59
done