Closed Bug 376919 Opened 14 years ago Closed 14 years ago

hidden pref to disable microsummary functionality

Categories

(Firefox Graveyard :: Microsummaries, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 3 alpha5

People

(Reporter: myk, Assigned: rflint)

References

Details

Attachments

(1 file)

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.
Attached patch PatchSplinter Review
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).
Assignee: nobody → ryan
Status: NEW → ASSIGNED
Attachment #261913 - Flags: review?(myk)
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.
Attachment #261913 - Flags: review?(myk) → review+
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.
Attachment #261913 - Flags: review+ → review?(myk)
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.
Attachment #261913 - Flags: review?(myk) → review+
Whiteboard: [checkin needed]
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → Firefox 3 alpha5
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.