799612 - Stop generating bug summary tooltips for linkified bugs, to reduce bzapi calls from the UI

Status: RESOLVED WONTFIX

(Tree Management Graveyard :: TBPL, defect)

Description

[Ed Morley [:emorley]]

_linkBugs() is used to generate tooltips on mouseover for bug status+summary.

However some of the places in which we use it are unnecessary and cause extra bzapi calls for little benefit. (For example the call from the tree status message is broken).

We should assess each use and remove unless it's actually adding something (which in the case of the pushlog, is not much, given the commit message is already shown).

We use it twice here, but broken:
{
      updateTreeStatus: function (data, staticSheriff) {
        var cssStatus = data.status;
        if (cssStatus == "approval required")
          cssStatus = "approvalRequired";
        var baseMessage = '<span class="treestatus ' + cssStatus + '">' +
                          data.status.toUpperCase() + '</span>. ' +
                          self._linkBugs(data.reason, false, false);
        if (data.reason)
          baseMessage += '. ';
        if (cssStatus == "open" && data.message_of_the_day)
          $('#tree-status').html(baseMessage + self._linkBugs(data.message_of_the_day, false, false));
        else
          $('#tree-status').html(baseMessage);
        if (staticSheriff)
          $("#sheriff").html(staticSheriff);
      },
}


and:
{
    var patchhtml = patches.map(function buildHTMLForPushPatch(patch, patchIndex) {
      return '<li>\n' +
      '<a class="revlink" data-rev="' + patch.rev + '" href="' + self._changesetURL(patch.rev) + '">' + patch.rev +
      '</a>\n<span class="patchTitle"><span class="author">' +
      patch.author.escapeContent() + '</span> &ndash; ' +
      '<span class="desc">' + self._linkBugs(patch.desc.split("\n")[0], false, true) + '</span>' +
      (function buildHTMLForPatchTags() {
        if (!patch.tags.length)
          return '';
        return ' <span class="logtags">' + $(patch.tags).map(function () {
          return ' <span class="' + this.type.escapeAttribute() + '">' + this.name.escapeContent() + '</span>';
        }).get().join('') + '</span>';
      })() +
      '</span>\n' +
      '</li>';
    });
}


and:
{
       (function htmlForPopup() {
         return '<div class="stars">' +
          (function htmlForNoteInPopup() {
            if (!self._machineResultHasNotes(result))
              return '';
            return '<div class="note">' +
              result.notes.map(function htmlForNote(note) {
                return '[<strong>' + note.who.escapeContent() + '</strong>' +
                  (note.timestamp ? (' - ' + UserInterface._getDisplayDate(new Date(note.timestamp * 1000))) : '') +
                  ']<br/>' + self._linkBugs(note.note, true, true);
                }).join("<br/><br/>") +
                '</div>';
          })() +
          (result.state == 'running' || result.state == 'pending' ? '' : '<div class="summary"><span id="summaryLoader"></span></div>') +
          '</div>';
      })();
}

Comment 1

[Ed Morley [:emorley]]

We should just get rid of them, I never stop to look at tooltips & most of them are on the pushlog and so redundant. TBPL is complex enough without extra rarely-used-shiny.

Comment 2

[Arpad Borsos [:Swatinem]]

I do not agree. There are a lot of commits that give no real summary apart from the bug number. Also the stars and the summary (you say it is broken?)

Comment 3

[Ed Morley [:emorley]]

Attachment: Patch v1

The linkified bugs are only used for:
* Pushlog
* Links shown in annotated summary after something has already been starred
* Tree status

a) Adding tooltips (containing bug summary) to the pushlog is redundant
b) Doubt anyone makes use of this
c) Tooltips in tree status are broken anyway
d) If we decide to keep this extra shiny, then bug 799614 means we'll have to include the bloat.

-> Let's just get rid of it for now.

Comment 4

[Ed Morley [:emorley]]

s/include/increase/

Comment 5

[Matt Brubeck (:mbrubeck)]

What if we just add a small (maybe 200ms) delay before loadBug is called on mouseover?  Then it won't generate dozens of useless bzapi calls just because you move your mouse across the page, but it will still work when you actually hover over a bug number.

Comment 6

[Ed Morley [:emorley]]

Swatinem, sorry I didn't get a mid-air on comment 2, only just seen it.

(In reply to Matt Brubeck (:mbrubeck) from comment #5)
> What if we just add a small (maybe 200ms) delay before loadBug is called on
> mouseover?  Then it won't generate dozens of useless bzapi calls just
> because you move your mouse across the page, but it will still work when you
> actually hover over a bug number.

That's bug 799614, which I filed as an alternative to this.

I just don't think people actually make use of this feature - I personally only just found out we even looked up the bug summary for the tooltip & I'm more familiar with TBPL than those that don't sheriff - and if many people actually used it, then the broken instances would have been reported already.

My main concern is us adding quite a lot (across all of the bug 799534 dependants) to the 50,000 bzapi calls that b.m.o has to field a day (I had a good chat with glob the other day about it; the whole bzapi story is pretty sadfaces, but I digress). If we really feel that enough people use this to make it worth keeping, then ok I can add the 200ms delay and fix up any usages. 

There's just so much else I want to do to TBPL (we have an awful lot of maintenance debt yet to pay) & it's already (IMHO) bloated in places as it is, that it just didn't seem worth hanging onto this.

Comment 7

[Matt Brubeck (:mbrubeck)]

Like Arpad I do use the tooltips (specifically for the "already starred" case), but I'm sure I could easily adapt if we decide to remove them.

Comment 8

[Ed Morley [:emorley]]

I think it doesn't help that I jsut find tooltips too slow to be useful in general. For the already-starred case, perhaps we should list a partial summary next to the bug number/"[OrangeFactor]" link? I'd find that useful.

Comment 9

[Arpad Borsos [:Swatinem]]

Comment on attachment 670428 [details] [diff] [review]
Patch v1

Review of attachment 670428 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review until we figure out a better solution than to just remove the tooltips altogether.

Comment 10

[Ed Morley [:emorley]]

Ok, of the following uses:

> * Links shown in annotated summary after something has already been starred
...kind of useful and not often triggered by people using TBPL.

> * Tree status
...useful - I've filed bug 805550 to fix.

> * Pushlog
...I'm dubious as to how useful these are for most users, but I'm happy to let it slide if you both use them - and with bug 799614 the impact will be much less.

As such, just going to wontfix this in favour of bug 799614 and bug 805550.