redesigning live title update code

[Myk Melez]

I noticed a few issues with the way the microsummary service updates
live titles, and I'm interested in feedback on a proposal for addressing
them...

Three code points prompt a live title to "update" (i.e.
retrieve/generate its microsummary, then save it to the datastore,
triggering XUL templates to rebuild the UI displaying it):

1. a repeating timer that checks for expired titles every 15 seconds;
2. the Add Bookmark and Bookmark Properties dialogs
(when a user selects a live title for the Name of a bookmark);
3. the Reload Live Title command on the bookmark context menu.

The flow looks something like this, where all functions except
microsummary.update() are methods of the MicrosummaryService:

repeating timer Reload Live Title bookmark dialogs
| | |
V | V
_updateMicrosummaries() | setMicrosummary(bookmarkID, microsummary)
| | |
| | V
| | [microsummary retrieved/generated?]
| | | |
| | no yes
| | | |
V V V |
refreshMicrosummary(bookmarkID) |
| |
V |
microsummary.update() |
(retrieve/generate) |
| |
V |
(observer) |
| |
V V
_updateMicrosummary(bookmarkID, microsummary)
(save to datastore, trigger UI rebuild)

There are a couple problems with this code:

1. refreshMicrosummary always instantiates a new Microsummary object,
even when it's called from setMicrosummary, which already has one.

2. The words "update", "refresh", and "set" have inconsistent meanings:

_updateMicrosummaries: update = retrieve/generate
refreshMicrosummary: refresh = retrieve/generate
microsummary.update(): update = retrieve/generate
setMicrosummary: set = save to datastore, trigger UI rebuild
_updateMicrosummary: update = save to datastore, trigger UI rebuild

And here's a set of proposed solutions:

1. Adopt a consistent naming convention in which "update" (or some other
term) means "retrieve/generate" and "set" (or some other term) means
"save to the datastore, trigging UI rebuilds".

2. Fold _updateMicrosummary into setMicrosummary, and have the observer
call setMicrosummary after microsummary.update finishes
retrieving/generating the microsummary.

3. Have setMicrosummary call microsummary.update directly instead of
calling refreshMicrosummary.

After these changes, the flow would look something like this:

repeating timer Reload Live Title bookmark dialogs
| | |
V | V
_updateMicrosummaries() | setMicrosummary(bookmarkID, microsummary)
| | (save to datastore, trigger UI rebuild)
| | | ^
| | microsummary |
| | not yet |
| | retrieved/ |
| | generated |
| | | |
V V | |
updateMicrosummary(bookmarkID) | |
| | |
V V |
microsummary.update() |
(retrieve/generate) |
| |
V |
(observer) |
| |
+---------------------+

Note 1: in this approach, to avoid getting stuck in an infinite loop,
the observer has to make sure the microsummary was successfully
retrieved/generated before calling setMicrosummary. That's easy to do,
but if we're worried about triggering such a loop via unforeseen bugs,
then we could just keep _updateMicrosummary (but rename it to something
else, like _setMicrosummary).

Note 2: another issue is that we use the word "microsummary" in many
places where we actually mean "live title" (i.e. the specific usage of
microsummaries as the titles of bookmarks). But I think it makes more
sense to address that separately if/when we start using microsummaries
in other contexts (f.e. as the labels for tabs).

Thoughts?

-myk