634915 - Add hooks to self-serve buildapi to tbpl to retrigger builds/test runs

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[(not currently active) Ted Mielczarek]

The self-serve buildapi (in the URL field) lets you kick off new builds/test runs. We should expose this via TBPL for better ease of use.

Comment 2

[Cameron McCormack (:heycam)]

I've got something working that can cancel existing builds, although because it requires a cross-domain request from tbpl.mozilla.org to build.mozilla.org, I can't check the output of the self-serve request to see whether it worked.  Can we add the CORS header to the self-serve API pages so that TBPL can access it fully?  (And I think we should do this from within the web page, rather than proxying it through one of TBPL's php scripts, since we want to use the HTTP auth credentials of the user for the self-serve request.)  If not, then it's not a great deal.

Comment 3

[Phil Ringnalda (:philor)]

That'd be bug 646636

Comment 4

[Cameron McCormack (:heycam)]

Chris, when I try to cancel a mochitest-5 run I get (if I inspect https://build.mozilla.org/buildapi/self-serve/jobs/1699) message "Not stopping unstoppable build" (and errors = false).  Are some kinds of jobs cancellable but others not?

Comment 5

[Chris AtLee [:catlee]]

(In reply to comment #4)
> Chris, when I try to cancel a mochitest-5 run I get (if I inspect
> https://build.mozilla.org/buildapi/self-serve/jobs/1699) message "Not stopping
> unstoppable build" (and errors = false).  Are some kinds of jobs cancellable
> but others not?

Yes, test jobs are unkillable for the moment due to bug 634592

Comment 6

[Cameron McCormack (:heycam)]

Attachment: Add hooks to self-serve buildapi to tbpl to retrigger builds/test runs (WIP v0.1)

Cancelling builds works, because that doesn't require looking at the output from the self-serve API; respinning builds doesn't yet.

Comment 7

[(no longer active)]

Do you have an instance somewhere with this patch applied so that we can test it?

Comment 8

[Cameron McCormack (:heycam)]

It's currently applied on http://tbpl.mcc.id.au/, but that's also where I'm directly making changes, so no guarantees that anything works there at any given time.

Comment 9

[(no longer active)]

(In reply to comment #8)
> It's currently applied on http://tbpl.mcc.id.au/, but that's also where I'm
> directly making changes, so no guarantees that anything works there at any
> given time.

That's true for a lot of things at Mozilla!  ;-)

Thanks, I'll try playing with it and will give you feedback.

Comment 10

[Cameron McCormack (:heycam)]

Attachment: Add hooks to self-serve buildapi to tbpl to retrigger builds/test runs (WIP v0.2)

Cancelling builds and respinning completed builds now works.

Todo:
* Verify that cancelling pending builds works; I'll need to wait until try is
  busy during the week to test that.
* Add the ability to respin more than one build at once.  (Maybe shift+clicking
  the "+" button should prompt for a number.)
* Find somewhere nicer to put the "Requesting build cancellation..." status
  message on the page.
* Encapsulate the self-serve API calls in a separate object.

Comment 11

[Cameron McCormack (:heycam)]

Attachment: Add hooks to self-serve buildapi to tbpl to retrigger builds/test runs (WIP v0.2.1)

Minor fixes for breakage I didn't test before uploading previous patch.

Comment 12

[(no longer active)]

(In reply to comment #10)
> * Verify that cancelling pending builds works; I'll need to wait until try is
>   busy during the week to test that.

Good idea.

> * Add the ability to respin more than one build at once.  (Maybe shift+clicking
>   the "+" button should prompt for a number.)

Great!  I was going to suggest that!

BTW, this doesn't need to happen in the same bug, as long as you disable the add button once it's clicked, until the adding operation finishes.

> * Find somewhere nicer to put the "Requesting build cancellation..." status
>   message on the page.

How about at the top center, where we display the rest of our progress notifications?

The only tricky part is that if canceling or rebuilding fails, we probably want to warn users in a very noticeable way...

> * Encapsulate the self-serve API calls in a separate object.

Also:

* Enable canceling all of the jobs for a push.

But this can happen in its own bug, too.

Comment 13

[Cameron McCormack (:heycam)]

Attachment: Add hooks to self-serve buildapi to tbpl to retrigger builds/test runs (WIP v0.3)

Messages about builds being cancelled/rebuilt are displayed in the top center of the page, where the "Loading..." messages go.  (Since these are now both displayed in the same place, and the messages can be a bit longer than the current ones, I've added some machinery to handle multiple messages being displayed simultaneously, and coloured that status box so that it doesn't blend in with the rest of the text behind/around it.)

I factored out the self-serve API calls into a BuildAPI object.

Kicking off multiple builds just by clicking the rebuild button a few times in a row works fine, so I think we don't need a prompt for the number of builds for now.  (It staggers them so that there's at least three seconds delay between each rebuild request.)

The "cancel all jobs for this push" can be left for a followup bug, as you say.

Comment 14

[Cameron McCormack (:heycam)]

Attachment: Add hooks to self-serve buildapi to tbpl to retrigger builds/test runs (v1)

(The last patch uploaded was bogus; I attached the wrong file.)

I verified that cancelling pending builds works.

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 526861 [details] [diff] [review]
Add hooks to self-serve buildapi to tbpl to retrigger builds/test runs (v1)

The patch is applied on http://tbpl.mcc.id.au/ if you wish to test it.

Comment 16

[Arpad Borsos [:Swatinem]]

Sorry, I'm on vacation for a week starting tomorrow morning so I don't have time putting thought into this.

Comment 17

[(no longer active)]

Comment on attachment 526861 [details] [diff] [review]
Add hooks to self-serve buildapi to tbpl to retrigger builds/test runs (v1)

>+  getUnfinishedMachineResult: function Data_getUnfinishedMachineResult(runID) {
>+    for (var repo in this._pendingJobs) {
>+      for (var rev in this._pendingJobs[repo]) {
>+        for (var i = 0; i < this._pendingJobs[repo][rev].length; i++) {
>+          var pendingJob = this._pendingJobs[repo][rev][i];
>+          if (pendingJob.id == runID) {
>+            return this._machineResultFromPendingOrRunningRun("pending", pendingJob, rev);
>+          }
>+        }
>+      }
>+    }
>+
>+    for (var repo in this._runningJobs) {
>+      for (var rev in this._runningJobs[repo]) {
>+        for (var i = 0; i < this._runningJobs[repo][rev].length; i++) {
>+          var runningJob = this._runningJobs[repo][rev][i];
>+          if (runningJob.id == runID) {
>+            return this._machineResultFromPendingOrRunningRun("running", runningJob, rev);
>+          }
>+        }
>+      }
>+    }
>+  },

Nit: it looks clear to have an internal function which makes the algorithm generic on a given jobs container, and then just call it twice, once for _pendingJobs and once for _runningJobs.

>-  crossDomainPost: function NetUtils_crossDomainPost(url, values, callback) {
>+  crossDomainPostWithCredentials: function NetUtils_crossDomainPostWithCredentials(url, requestHeaders, values, loadCallback, failCallback, timeoutCallback, timeout) {
>+    function hex(i) {
>+      return (i < 16 ? '0' : '') + i.toString(16);
>+    }
>+
>+    function enc(s) {
>+      // Encodes s as an application/x-www-form-urlencoded string.
>+      return String(s).replace(/[^\0-\x7f]/g, function(c) { return '&#' + c.charCodeAt(0) + ';' })
>+                      .replace(/[^ 0-9A-Za-z$_.!*'()-]/g, function(c) { return '%' + hex(c) })
>+                      .replace(/ /g, '+');
>+    }

I'm not an encoding wizard, but this seems wrong, since application/x-www-form-urlencoded fields are not supposed to have ampersand characters (since those characters are used as name/value pair separators).

I'm wondering if this wouldn't do all that you need:

function enc(s) {
  return escape(s).replace('%20', '+');
}

>+    var headers = requestHeaders ? Controller.copyObject(requestHeaders) : { };
>+    headers['Content-Type'] = 'application/x-www-form-urlencoded;charset=US-ASCII';

Why the us-ascii charset?

>+    $("#messages").bind("click", function messagesClick() {
>+      self.hideMessages(true);
>+    });

Is this needed?  I don't want those messages to go away if I accidentally click on them.  I think we should just force everybody to see them all the time, given that there is already an obvious escape route (reloading).

>+  _loadingMessagesShowing: function UserInterface__loadingMessagesShowing() {
>+    for (var e = document.getElementById("messages").firstChild; e; e = e.nextSibling) {
>+      if (e.className == "loading") {
>+        return true;
>+      }
>+    }
>+    return false;

Is this what you meant?  :-)

return document.getElementById("messages").querySelectorAll(".loading").length > 0;

>+  _makeStatusCallback: function UserInterface__makeStatusCallback(mid, s, fn) {
>+    var self = this;
>+    return function statusCallback(e) {
>+      if (e == '[object ProgressEvent]') {

This will break in debug builds, since the string version of the event object would be something like "[object ProgressEvent 0xdeadbeef]".  You can use e.toString().indexOf() instead.

>+  _rebuildButtonClick: function UserInterface__rebuildButtonClick(rebuildButton, runID) {
>+    var result = this._data.getMachineResult(runID);
>+    var desc = result.getShortDescription();
>+
>+    var mid = this.showMessage('Requesting rebuild of ' + desc + 'â¦', true);
>+    var onSuccess = this._makeStatusCallback(mid, 'Rebuild of ' + desc + ' requested.');
>+    var onTimeout = this._makeStatusCallback(mid, '<span class="failed">Rebuild request for ' + desc + ' timed out.</span>');
>+    var onFailure = this._makeStatusCallback(mid, '<span class="failed">Rebuild request for ' + desc + ' failed.</span>');
>+
>+    // Leave at least 3 seconds between rebuild requests.
>+    var now = Date.now();
>+    var delay = Math.max(this._nextBuildRequest - now, 0);
>+    this._nextBuildRequest = now + delay + 3000;
>+    setTimeout(function() {
>+      result.getBuildIDForSimilarBuild(
>+        function rebuildButtonClick_GotBuildID(buildID) {
>+          try {
>+            BuildAPI.rebuild(result.getBuildbotBranch(), buildID, onSuccess, onFailure, onTimeout);
>+          } catch (e) {
>+            onFailure(e);
>+          }
>+        },
>+        onFailure, onTimeout);
>+    }, delay);
>+  },

I'm wondering if we need to deal with failures better.  I mean, it's nice to see a progress of the rebuild request, but if the result is a failure, I don't want to miss it if I'm not looking at the screen.  Can we get them to persist, in a color which grabs one's attention?

>+  _cancelButtonClick: function UserInterface__cancelButtonClick(cancelButton, requestOrBuildID) {

Ditto here, although it's less important when canceling a build.

>+        return '<div id="results">' +
>+          (testResults.length ? 
>+            '<ul>\n' +
>+            testResults.map(function htmlForTestResultEntry(r) {
>+              return '<li>' + r.name +
>+                (r.result ? ': ' + (r.resultURL ? '<a href="' + r.resultURL.replace(/"/g, "&quot;") +

Are we only worried about quotes here?

I know that we're doing this in a bunch of places already, but I think we should stop, and get a proper htmlEncode method or something.

>+                                                  '">' + r.result + '</a>'
>+                                                : r.result)
>                       : '') +
>-            (r.detailsURL ? ' (<a href="' + r.detailsURL.replace(/"/g, "&quot;") +
>-                            '">details</a>)'
>-                          : '') +
>-            '</li>';
>-        }).join("") +
>-        '</ul>';
>+                (r.detailsURL ? ' (<a href="' + r.detailsURL.replace(/"/g, "&quot;") +

Likewise here.

This patch looks great to me with the above addressed, but I think Markus should also take a look.  But I have to say, I was seriously tempted to SHIP IT as it is!  :-)

Comment 18

[Cameron McCormack (:heycam)]

(In reply to comment #17)
> Nit: it looks clear to have an internal function which makes the algorithm
> generic on a given jobs container, and then just call it twice, once for
> _pendingJobs and once for _runningJobs.

Fair.

> >+    function enc(s) {
> >+      // Encodes s as an application/x-www-form-urlencoded string.
> >+      return String(s).replace(/[^\0-\x7f]/g, function(c) { return '&#' + c.charCodeAt(0) + ';' })
> >+                      .replace(/[^ 0-9A-Za-z$_.!*'()-]/g, function(c) { return '%' + hex(c) })
> >+                      .replace(/ /g, '+');
> >+    }
> 
> I'm not an encoding wizard, but this seems wrong, since
> application/x-www-form-urlencoded fields are not supposed to have ampersand
> characters (since those characters are used as name/value pair separators).

According to http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#url-encoded-form-data characters that can't be represented in the encoding being used get represented as strings like "&#nnn;", and those characters then get percent encoded.

> I'm wondering if this wouldn't do all that you need:
> 
> function enc(s) {
>   return escape(s).replace('%20', '+');
> }

I couldn't tell from https://developer.mozilla.org/en/DOM/window.escape exactly what escape does.  It seems escape("\u4e0a") == "%u4E0A", which I can't see as being allowed in the application/x-www-form-urlencoded definition.

> >+    var headers = requestHeaders ? Controller.copyObject(requestHeaders) : { };
> >+    headers['Content-Type'] = 'application/x-www-form-urlencoded;charset=US-ASCII';
> 
> Why the us-ascii charset?

I was going for simplicity (encoding the form data ASCII), but I think choosing UTF-8 will be simpler, then I won't need to do the "&#nnn;" business; let's just assume that the server will know how to deal with it.  (I don't think non-ASCII characters are going to be sent by the current uses of these NetUtils functions anyway.)

> >+    $("#messages").bind("click", function messagesClick() {
> >+      self.hideMessages(true);
> >+    });
> 
> Is this needed?  I don't want those messages to go away if I accidentally click
> on them.  I think we should just force everybody to see them all the time,
> given that there is already an obvious escape route (reloading).

Given the hard coded 8 second delay in hiding that box, I wanted to give a way to dismiss it earlier.  I'll remove it.

> >+  _loadingMessagesShowing: function UserInterface__loadingMessagesShowing() {
> >+    for (var e = document.getElementById("messages").firstChild; e; e = e.nextSibling) {
> >+      if (e.className == "loading") {
> >+        return true;
> >+      }
> >+    }
> >+    return false;
> 
> Is this what you meant?  :-)
> 
> return document.getElementById("messages").querySelectorAll(".loading").length
> > 0;

That works. Or:

  return document.querySelectorAll("#messages .loading").length > 0;

:)

> >+      if (e == '[object ProgressEvent]') {
> 
> This will break in debug builds, since the string version of the event object
> would be something like "[object ProgressEvent 0xdeadbeef]".  You can use
> e.toString().indexOf() instead.

Good point.  (I am kind of surprised that that doesn't break web pages.)

...
> I'm wondering if we need to deal with failures better.  I mean, it's nice to
> see a progress of the rebuild request, but if the result is a failure, I don't
> want to miss it if I'm not looking at the screen.  Can we get them to persist,
> in a color which grabs one's attention?

They are shown in red at the moment, but you are right they will disappear after 8 seconds and you might miss it.

How about failure messages are persistent in that status box, but get a little "x" next to them to dismiss them?

> >+  _cancelButtonClick: function UserInterface__cancelButtonClick(cancelButton, requestOrBuildID) {
> 
> Ditto here, although it's less important when canceling a build.

Yeah.

> >+        return '<div id="results">' +
> >+          (testResults.length ? 
> >+            '<ul>\n' +
> >+            testResults.map(function htmlForTestResultEntry(r) {
> >+              return '<li>' + r.name +
> >+                (r.result ? ': ' + (r.resultURL ? '<a href="' + r.resultURL.replace(/"/g, "&quot;") +
> 
> Are we only worried about quotes here?

We should be worrying about more than just quotes.  (This was just a reindenting of existing code, mind.)

> I know that we're doing this in a bunch of places already, but I think we
> should stop, and get a proper htmlEncode method or something.

I'll do that.

> This patch looks great to me with the above addressed, but I think Markus
> should also take a look.  But I have to say, I was seriously tempted to SHIP IT
> as it is!  :-)

;)

Comment 19

[Cameron McCormack (:heycam)]

(In reply to comment #18)
> > Why the us-ascii charset?
> 
> I was going for simplicity (encoding the form data ASCII), but I think choosing
> UTF-8 will be simpler, then I won't need to do the "&#nnn;" business; let's
> just assume that the server will know how to deal with it.  (I don't think
> non-ASCII characters are going to be sent by the current uses of these NetUtils
> functions anyway.)

Correction: using UTF-8 is *more* complicated, because what you need to do is convert each character that requires escaping into the sequence of bytes in UTF-8, and then encodes those bytes using "%nn".  For example, if you are sending the key-value pair ("hi", "I ♥ 上海") with UTF-8 as the selected encoding, it should be encoded as

  hi=I+%e2%99%a5+%e4%b8%8a%e6%b5%b7

whereas if you're using ASCII, it would be:

  hi=I+%26%239829%3b+%26%2319978%3b%26%2328023%3b

Longer, but easier to generate than having to write a UTF-8 encoder!

Comment 20

[Cameron McCormack (:heycam)]

Attachment: Add hooks to self-serve buildapi to tbpl to retrigger builds/test runs (v2)

Addressed Ehsan's feedback.

I added functions to do the markup escaping for attributes and text content, and fixed the above quoted instances of it, but I didn't ensure escape is done correctly in the rest of the file(s).

Comment 21

[(no longer active)]

(In reply to comment #18)

I stand corrected on the encoding stuff!  :-)

> > >+    var headers = requestHeaders ? Controller.copyObject(requestHeaders) : { };
> > >+    headers['Content-Type'] = 'application/x-www-form-urlencoded;charset=US-ASCII';
> > 
> > Why the us-ascii charset?
> 
> I was going for simplicity (encoding the form data ASCII), but I think choosing
> UTF-8 will be simpler, then I won't need to do the "&#nnn;" business; let's
> just assume that the server will know how to deal with it.  (I don't think
> non-ASCII characters are going to be sent by the current uses of these NetUtils
> functions anyway.)

We can continue to use us-ascii for now.  I was just curious to see if there is a particular reason why we're not using UTF-8 (and I now know that there is!).

I think nobody cares in practice for now.

> > Is this what you meant?  :-)
> > 
> > return document.getElementById("messages").querySelectorAll(".loading").length
> > > 0;
> 
> That works. Or:
> 
>   return document.querySelectorAll("#messages .loading").length > 0;
> 
> :)

Indeed!  There is a shorter jQuery syntax format which I don't remember, but this is good enough.  Web APIs FTW!

> > >+      if (e == '[object ProgressEvent]') {
> > 
> > This will break in debug builds, since the string version of the event object
> > would be something like "[object ProgressEvent 0xdeadbeef]".  You can use
> > e.toString().indexOf() instead.
> 
> Good point.  (I am kind of surprised that that doesn't break web pages.)

It does.  This is one of the main reasons why a website wouldn't work in debug builds.  And the only reason I know this is because I did the same thing in Bugzilla Tweaks, and Boris caught me.  :-)

> How about failure messages are persistent in that status box, but get a little
> "x" next to them to dismiss them?

That sounds like a perfect solution to me.

> We should be worrying about more than just quotes.  (This was just a
> reindenting of existing code, mind.)

Heh, then shame on me for missing that!  Fair enough.

> > I know that we're doing this in a bunch of places already, but I think we
> > should stop, and get a proper htmlEncode method or something.
> 
> I'll do that.

Thanks!

> > This patch looks great to me with the above addressed, but I think Markus
> > should also take a look.  But I have to say, I was seriously tempted to SHIP IT
> > as it is!  :-)
> 
> ;)

/me is still resisting the urge of just landing this patch and deploying it right now...

Comment 22

[Arpad Borsos [:Swatinem]]

Comment on attachment 527178 [details] [diff] [review]
Add hooks to self-serve buildapi to tbpl to retrigger builds/test runs (v2)

Review of attachment 527178 [details] [diff] [review]:

tango-media-playback-stop.png doesn’t seem to be used. Also the new bugzilla review plugin does not list which binary files are changed, that is quite annoying.

Very nice patch indeed. r+ with nits fixed.

::: css/style.css
@@ +772,5 @@
+  border: 1px solid #ddd;
+  border-top: none;
+  background: rgba(255, 255, 192, 0.8);
+  padding: 4px;
+  display: table;

why table?

::: images/closebutton-dark.svg
@@ +1,4 @@
+<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 1 16 16" shape-rendering="geometricPrecision">
+  <circle cx="8" cy="8" r="6" fill="#aaa"/>
+  <path d="M5,5 L11,11 M11,5 L5,11" fill="none" stroke="white" stroke-width="2"/>
+</svg>

This looks very similar to the comment-ui-close button, may be worth unifying those in a followup.

::: js/Controller.js
@@ +40,5 @@
 
+  escapeContent: function Controller_escapeContent(text) {
+    return text.replace(/&/g, "&amp;")
+               .replace(/</g, "&lt;");
+  },

Isn’t this basically the same as stripTags above? Why don’t we need to strip the > in escapeContent?

@@ +47,5 @@
+    return this.escapeContent(text)
+               .replace(/>/g, "&gt;")
+               .replace(/"/g, "&quot;")
+               .replace(/'/g, "&apos;");
+  },

Note to myself: moving these helper functions into its own file makes sense.

::: js/Data.js
@@ +417,5 @@
+      var jobs = self["_" + status + "Jobs"];
+      for (var repo in jobs) {
+        for (var rev in jobs[repo]) {
+          for (var i = 0; i < jobs[repo][rev].length; i++) {
+            var pendingJob = jobs[repo][rev][i];

The naming is wrong for the generic function here, just call it “job”.

@@ +608,5 @@
+      return "b-c";
+
+    return "";
+  },
+};

Perfect, thanks for moving this code here, it’s a much better place than UI.js

::: js/NetUtils.js
@@ +41,2 @@
     } catch (e) {
+      clearTimeout(errorTimer);

try&catch is sync as far as I know, so errorTimer is undefined here. Move the definition of errorTimer above the try&catch.

@@ +57,5 @@
+      return (i < 16 ? '0' : '') + i.toString(16);
+    }
+
+    function enc(s) {
+      // Encodes s as a US-ASCII application/x-www-form-urlencoded string.

Well I’m one of the rare breed who dream of a world where there is only one encoding to rule them all: UTF8.
But having read your discussion, I think we are not quite there yet so I might just let this one through :-)

::: js/UserInterface.js
@@ +163,5 @@
     }
+
+    function showStatusMessage(midName, text, type) {
+      if (!self._statusMessageIDs[midName]) {
+        var mid = self.showMessage(text, type);

Maybe it’s just me, when I read “mid” I always think of “middle”. Spell out messageid. Also, “type” may be better than “name”.

@@ +1052,5 @@
+    var messages = document.getElementById("messages");
+    messages.style.display = "none";
+    while (messages.firstChild) {
+      messages.removeChild(messages.firstChild);
+    }

$("#messages").hide().empty();

@@ +1065,5 @@
+      return;
+    }
+
+    var self = this;
+    this._messageTimer = setTimeout(function() { self._messageTimer = 0; self.hideMessages() }, 8000);

Please properly indent the closure.

@@ +1083,5 @@
+    }
+
+    messages.style.display = 'table';
+    div.className = type || '';
+    div.innerHTML = (type == 'error' ? '<a class="messageDismiss" href="#" onclick="UserInterface.updateMessage(\'' + div.id + '\', \'\'); return 0"></a>'

Something is wrong with that return 0, it added a # to my querystring when I tested this.

@@ +1306,5 @@
     if (result.note)
       box.addClass("hasStar");
     box.html((function htmlForResultInBottomBar() {
       var revs = result.revs;
+      var desc = Config.OSNames[result.machine.os] + (result.machine.debug ? ' debug ' : ' opt ') + result.machine.type;

You have added a function for that, haven’t you?

Comment 23

[Jeff Walden [:Waldo]]

(In reply to comment #19)
> Correction: using UTF-8 is *more* complicated, because what you need to do is
> convert each character that requires escaping into the sequence of bytes in
> UTF-8, and then encodes those bytes using "%nn".  For example, if you are
> sending the key-value pair ("hi", "I ♥ 上海") with UTF-8 as the selected
> encoding, it should be encoded as
> 
>   hi=I+%e2%99%a5+%e4%b8%8a%e6%b5%b7
> 
> whereas if you're using ASCII, it would be:
> 
>   hi=I+%26%239829%3b+%26%2319978%3b%26%2328023%3b
> 
> Longer, but easier to generate than having to write a UTF-8 encoder!

Would encodeURIComponent do the trick?  Trying it just here I see it escapes spaces differently, but would they process to the same thing in whatever's parsing the data?

Comment 24

[Cameron McCormack (:heycam)]

Thanks for the review comments!

(In reply to comment #22)
> tango-media-playback-stop.png doesn’t seem to be used.

Removed.

> ::: css/style.css
> @@ +772,5 @@
> +  border: 1px solid #ddd;
> +  border-top: none;
> +  background: rgba(255, 255, 192, 0.8);
> +  padding: 4px;
> +  display: table;
> 
> why table?

I couldn't seem to get the behaviour I wanted (width determined by the box's content, with left and right margins auto so that the box is centered).  I rejigged the styles a bit and got it working with inline-block now, though.

> ::: images/closebutton-dark.svg
> This looks very similar to the comment-ui-close button, may be worth unifying
> those in a followup.

Yes, I tried to use that image first but it was too light against the colour of the message panel.

> ::: js/Controller.js
> @@ +40,5 @@
> 
> +  escapeContent: function Controller_escapeContent(text) {
> +    return text.replace(/&/g, "&")
> +               .replace(/</g, "&lt;");
> +  },
> 
> Isn’t this basically the same as stripTags above? Why don’t we need to strip
> the > in escapeContent?

Yeah, actually I should just remove one.  The only use of stripTags is to escape some text to be used as content.  ">" doesn't need to be escaped in content, just in attribute values.

> @@ +47,5 @@
> +    return this.escapeContent(text)
> Note to myself: moving these helper functions into its own file makes sense.

There are probably perfectly good functions in JQuery to do this, too, but I'm not very familiar with it.

> ::: js/Data.js
> @@ +417,5 @@
> +      var jobs = self["_" + status + "Jobs"];
> +      for (var repo in jobs) {
> +        for (var rev in jobs[repo]) {
> +          for (var i = 0; i < jobs[repo][rev].length; i++) {
> +            var pendingJob = jobs[repo][rev][i];
> 
> The naming is wrong for the generic function here, just call it “job”.

ack.

> ::: js/NetUtils.js
> @@ +41,2 @@
>      } catch (e) {
> +      clearTimeout(errorTimer);
> 
> try&catch is sync as far as I know, so errorTimer is undefined here. Move the
> definition of errorTimer above the try&catch.

I think I'll just remove the clearTimeout call, it's unnecessary there.

> @@ +57,5 @@
> +      // Encodes s as a US-ASCII application/x-www-form-urlencoded string.
> 
> Well I’m one of the rare breed who dream of a world where there is only one
> encoding to rule them all: UTF8.

I think you are not alone in that! :)

> ::: js/UserInterface.js
> @@ +163,5 @@
>      }
> +
> +    function showStatusMessage(midName, text, type) {
> +      if (!self._statusMessageIDs[midName]) {
> +        var mid = self.showMessage(text, type);
> 
> Maybe it’s just me, when I read “mid” I always think of “middle”. Spell out
> messageid. Also, “type” may be better than “name”.

Since there's already an argument "type", I've renamed them all to (statusType, messageText, messageType).

> @@ +1052,5 @@
> +    var messages = document.getElementById("messages");
> +    messages.style.display = "none";
> +    while (messages.firstChild) {
> +      messages.removeChild(messages.firstChild);
> +    }
> 
> $("#messages").hide().empty();

That is shorter. :-)

> @@ +1065,5 @@
> +      return;
> +    }
> +
> +    var self = this;
> +    this._messageTimer = setTimeout(function() { self._messageTimer = 0;
> self.hideMessages() }, 8000);
> 
> Please properly indent the closure.

OK.

> @@ +1083,5 @@
> onclick="UserInterface.updateMessage(\'' + div.id + '\', \'\'); return 0"></a>'
> 
> Something is wrong with that return 0, it added a # to my querystring when I
> tested this.

I changed it to `return false`.

> @@ +1306,5 @@
>      if (result.note)
>        box.addClass("hasStar");
>      box.html((function htmlForResultInBottomBar() {
>        var revs = result.revs;
> +      var desc = Config.OSNames[result.machine.os] + (result.machine.debug ? '
> debug ' : ' opt ') + result.machine.type;
> 
> You have added a function for that, haven’t you?

Actually that line can go altogether, since desc isn't used.

> Very nice patch indeed. r+ with nits fixed.

Thank you!

Comment 25

[Cameron McCormack (:heycam)]

(In reply to comment #23)
> Would encodeURIComponent do the trick?

It looks like it's close.

> Trying it just here I see it escapes spaces differently, but would they
> process to the same thing in whatever's parsing the data?

I am not sure.  Probably?

There are two other differences between encodeURIComponent and the application/x-www-form-urlencoded encoding algorithm from HTML5.  In the former, "~" can be used unescaped and "$" cannot, but this is reversed in the HTML5 definition.

Comment 26

[Arpad Borsos [:Swatinem]]

(In reply to comment #24)
> > ::: js/Controller.js
> > @@ +40,5 @@
> > 
> > +  escapeContent: function Controller_escapeContent(text) {
> > +    return text.replace(/&/g, "&")
> > +               .replace(/</g, "&lt;");
> > +  },
> > 
> > Isn’t this basically the same as stripTags above? Why don’t we need to strip
> > the > in escapeContent?
> 
> Yeah, actually I should just remove one.  The only use of stripTags is to
> escape some text to be used as content.  ">" doesn't need to be escaped in
> content, just in attribute values.

As far as I remember you also needed to escape > in content.

> > ::: js/NetUtils.js
> > @@ +41,2 @@
> >      } catch (e) {
> > +      clearTimeout(errorTimer);
> > 
> > try&catch is sync as far as I know, so errorTimer is undefined here. Move the
> > definition of errorTimer above the try&catch.
> 
> I think I'll just remove the clearTimeout call, it's unnecessary there.

Hm, in case of going through the catch, you end up creating the timer after you cancel it, that seems wrong, so creating the timer before is the better solution I guess.

Comment 27

[Cameron McCormack (:heycam)]

(In reply to comment #26)
> As far as I remember you also needed to escape > in content.

I'm pretty sure you don't need to in HTML:

  http://www.whatwg.org/specs/web-apps/current-work/multipage/tokenization.html#data-state

That's the normal tokenizer state for character data in content.

> Hm, in case of going through the catch, you end up creating the timer after you
> cancel it, that seems wrong, so creating the timer before is the better
> solution I guess.

I'm not following.  If you get into the catch, then it'll return before the timer is created.

Comment 28

[Arpad Borsos [:Swatinem]]

(In reply to comment #27)
> I'm not following.  If you get into the catch, then it'll return before the
> timer is created.

Yeah right, my bad, I overlooked that return somehow.

Comment 29

[Cameron McCormack (:heycam)]

Landed: http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/ba73d1e5789f

If someone could pull on to tbpl.mozilla.org that'd be great. :-)

Comment 30

[Cameron McCormack (:heycam)]

It needed a typo fix, too: http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/7ba7f5413310

Comment 31

[Cameron McCormack (:heycam)]

Ehsan deployed it.

Comment 32

[Phil Ringnalda (:philor)]

I pushed http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/79e8977336c1 since it accidentally reverted half of bug 647988, and made mobile browser-chrome invisible (which did rather nicely green up their tree, what with it being permaorange).

Comment 33

[Cameron McCormack (:heycam)]

Oops, thanks.

Comment 34

[Phil Ringnalda (:philor)]

nthomas deployed 79e8977336c1, while proving that he could.

Comment 35

[(not currently active) Ted Mielczarek]

Did I mention that this is like the best thing ever? Good work!

Comment 36

[Jeff Walden [:Waldo]]

Did I mention that this is like the best thing ever? Good work!