Make UserInterface able to regenerate an arbitrary push and to insert it at the correct position if it's new.

Comment on attachment 497892 [details] [diff] [review] part 1, v1 >+ _buildHTMLForPushPatches: function UserInterface__buildHTMLForPushPatches(push) { Honestly, I don’t see a reason for this method. The patches of a push won’t ever change. >- var nodeHtml = '<li id="push-' + push.patches[0].rev + '">\n' + >+ var nodeHtml = '<li class="push" id="push-' + push.id + '" data-id="' + push.id + '">\n' + Our push objects don’t have an id member (yet). Is this mixed up with some other changes in your queue? Instead of push.patches[0].rev you can use push.toprev Also, the class and data-id don’t seem to be in use yet. >+ _refreshPushResultsInPushNode: function UserInterface__refreshPushResultsInPushNode(push, node) { >+ $(".results", node).html(this._buildHTMLForPushResults(push)); >+ }, You could make use of this method instead of the if (exists) $(exists).replaceWith(self._generatePushNode(push)); blocks in the update and history load codepaths, that way we can avoid recreating some of the DOM.

Don't pass machines to the success callback. Get them via a public method on Data when needed.

Comment on attachment 497900 [details] [diff] [review] part 2, v1 >+ var currentPushNodeID = currentPushNode.attr("data-id"); >+ if (currentPushNodeID === undefined) >+ break; Why do we need the data attribute at all? Isn’t the id enough? And why the undefined check? .push children of #pushes are guaranteed to have id == data-id, don’t they? >+ _getPushNode: function UserInterface__getPushNode(push) { >+ return $("#push-" + push.id); >+ }, I’m not a fan of adding an indirection for such a trivial function.

Comment on attachment 497892 [details] [diff] [review] part 1, v1 It all makes sense now :)

(In reply to comment #2) > Comment on attachment 497892 [details] [diff] [review] > part 1, v1 > > >+ _buildHTMLForPushPatches: function UserInterface__buildHTMLForPushPatches(push) { > > Honestly, I don’t see a reason for this method. The patches of a push won’t > ever change. Factoring it out makes _generatePushNode shorter and easier to understand, don't you think? > Instead of push.patches[0].rev you can use push.toprev Good catch!

(In reply to comment #4) > Comment on attachment 497900 [details] [diff] [review] > part 2, v1 > > >+ var currentPushNodeID = currentPushNode.attr("data-id"); > >+ if (currentPushNodeID === undefined) > >+ break; > > Why do we need the data attribute at all? Isn’t the id enough? Which id are you referring to? > And why the > undefined check? .push children of #pushes are guaranteed to have id == > data-id, don’t they? Good point, I wonder why I added that. > >+ _getPushNode: function UserInterface__getPushNode(push) { > >+ return $("#push-" + push.id); > >+ }, > > I’m not a fan of adding an indirection for such a trivial function. Agreed. I thought I was going to use it more often, but I'm not.

Well ok, the difference is that id has a "push-" prefix and data-id has not. But a string.replace() should easily take care of that.

I prefer an extra attribute to a string.replace(). Landed parts 1 - 3: http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/7c22dc3bc0fb http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/b5e19f91bde8 http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/87c19733a945

Actually, the other parts that are coming don't belong to this bug because they don't have much to do with HTML generation. This is fixed.