369646 - refactor code to update live titles

Status: RESOLVED WONTFIX

(Firefox Graveyard :: Microsummaries, defect, P4)

Description

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

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).

Comment 1

[Marco Bonardo [:mak]]

- BUGSPAM -
Wontfixing all Microsummaries bugs, since the feature has been removed from the core product and previous versions won't get further fixes for it.
If interested in supporting Microsummaries in your add-on, you're free to use our old microsummaries code and to search all previously open bugs by looking for [microsummaries-feature-removal] in the status whiteboard field.