Continuously update status in the buildbot extension popup window

Continuously update status in the buildbot extension popup window

Add support for updating displayed status while the popup window is open, for
both builders and CL try jobs. Encapsulate issue state and sending events on
issue add/update/remove, which are listened for in the popup.

Also, initialize the username for try job monitoring by scraping Rietveld.


BUG=178588
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213334

[Mike Wittman, 2013-07-01 23:28:16]

Hi Benjamin, please take a look at the next set of changes to the buildbot
extension.

[not at google - send to devlin, 2013-07-02 20:55:57]

I can see that you've pushed a couple more revisions since I started reviewing
this yesterday - I'll have a look at whatever the change endsup being after
these comments.

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/examples/extensions/buildbot/bg.js (right):

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/bg.js:188:
activeIssues.issues().forEach(function(issue) {
you could also provide a forEach method on activeIssues.

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/bg.js:200: if
(username && username.trim()) {
what is the username.trim() for? empty-string is true...

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/bg.js:213: // Give
prefs a chance to be written, then try again.
I really hope this isn't necessary. prefs use storage API right?
- if you set then get, the new value will be returned
- if that's not what's going on, can you cache the last value within the prefs
class?

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/examples/extensions/buildbot/issues.js
(right):

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/issues.js:3: // found
in the LICENSE file.
should this file be active_issues.js?

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/issues.js:14:
array.push(this.issues_[key]);
it's good practice to check this.issues_.hasOwnProperty(key) here, though if you
use forEach that will do it for you.

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/issues.js:25: if
(this.issues_.hasOwnProperty(issue.issue))
how can this condition be false?

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/issues.js:47: }
see comment elsewhere about surrounding in (function() {}). Also, why the need
for the separate state object? Can't the ActiveIssue object own it itself?
Something like:

var background = chrome.extension.getBackgroundPage();
background.buildbot = background.buildbot || {};
background.buildbot.activeIssues_ =
    background.buildbot.activeIssues_ || new ActiveIssues();
window.buildbot = window.buildbot || {};
window.buildbot.getActiveIssues = function() {
  return background.activeIssues_;
};

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/examples/extensions/buildbot/popup.html
(right):

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/popup.html:53: <tr
id="post-cl-spacer" class="spacer"></tr>
can you implement this with CSS rather than a redundant DOM element? I commented
on this elsewhere, but setting a padding-bottom on the previous row would do it.

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/examples/extensions/buildbot/popup.js
(right):

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/popup.js:283:
table.insertRow(-1).insertCell().className = "spacer";
pertaining to the CSS comment; perhaps you can apply a padding-bottom to the
last row that you added.

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/popup.js:298: if
(request.event == "issueUpdated" || request.event == "issueAdded")
you can use switch here. however, using onMessage like this causes a round trip
to the browser which is unnecessary given the popup and the background page are
in the same process. you could set up a standard listener infrastructure
instead?

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/popup.js:307:
botStatusElements.item(i).src = botStatusElements.item(i).src;
so this causes the iframe to reload? wow... add a comment?

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/popup.js:311: main();
can you surround this whole file in an anonymous function? same for all other
files. if you want to export something then do it manually.

[Mike Wittman, 2013-07-03 22:53:32]

I think these changes should hit all the comments.

I also fixed an issue where there was no notification that the issue had been
updated after the deferred loading of try job state. This caused the try job
state to not be shown in the popup on mouseover.

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/examples/extensions/buildbot/bg.js (right):

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/bg.js:188:
activeIssues.issues().forEach(function(issue) {
On 2013/07/02 20:55:57, kalman wrote:
> you could also provide a forEach method on activeIssues.

Good idea. Done.

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/bg.js:200: if
(username && username.trim()) {
On 2013/07/02 20:55:57, kalman wrote:
> what is the username.trim() for? empty-string is true...

Actually empty string is false. :) My intention is to honor a blank username
preference set by the user by neither fetching try jobs nor scraping from
Rietveld. Checking username.trim() allows this even if the user has entered some
spaces in the text box for the username.

I've added a clarifying comment.

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/bg.js:213: // Give
prefs a chance to be written, then try again.
On 2013/07/02 20:55:57, kalman wrote:
> I really hope this isn't necessary. prefs use storage API right?
> - if you set then get, the new value will be returned
> - if that's not what's going on, can you cache the last value within the prefs
> class?

I wasn't sure if the ordering was preserved between gets and sets. Updated to
not use setTimeout.

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/examples/extensions/buildbot/issues.js
(right):

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/issues.js:3: // found
in the LICENSE file.
On 2013/07/02 20:55:57, kalman wrote:
> should this file be active_issues.js?

Probably. My intention was to introduce an Issues class here, but ActiveIssues
will still be the top-level class. Done.

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/issues.js:14:
array.push(this.issues_[key]);
On 2013/07/02 20:55:57, kalman wrote:
> it's good practice to check this.issues_.hasOwnProperty(key) here, though if
you
> use forEach that will do it for you.

Done.

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/issues.js:25: if
(this.issues_.hasOwnProperty(issue.issue))
On 2013/07/02 20:55:57, kalman wrote:
> how can this condition be false?

It can't. Addressed in the second patchset. :)

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/issues.js:47: }
On 2013/07/02 20:55:57, kalman wrote:
> see comment elsewhere about surrounding in (function() {}). Also, why the need
> for the separate state object? Can't the ActiveIssue object own it itself?
> Something like:
> 
> var background = chrome.extension.getBackgroundPage();
> background.buildbot = background.buildbot || {};
> background.buildbot.activeIssues_ =
>     background.buildbot.activeIssues_ || new ActiveIssues();
> window.buildbot = window.buildbot || {};
> window.buildbot.getActiveIssues = function() {
>   return background.activeIssues_;
> };

The separate state object is so that the background and popup page can share
data without the popup page invoking functions the background page. I tried
something similar to what you propose and getIssue at least returned undefined
when invoked from the popup.

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/examples/extensions/buildbot/popup.html
(right):

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/popup.html:53: <tr
id="post-cl-spacer" class="spacer"></tr>
On 2013/07/02 20:55:57, kalman wrote:
> can you implement this with CSS rather than a redundant DOM element? I
commented
> on this elsewhere, but setting a padding-bottom on the previous row would do
it.

Done; I used padding-top on the row below so I don't have to keep track the last
CL row as CLs get added/removed.

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/examples/extensions/buildbot/popup.js
(right):

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/popup.js:283:
table.insertRow(-1).insertCell().className = "spacer";
On 2013/07/02 20:55:57, kalman wrote:
> pertaining to the CSS comment; perhaps you can apply a padding-bottom to the
> last row that you added.

Done.

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/popup.js:298: if
(request.event == "issueUpdated" || request.event == "issueAdded")
On 2013/07/02 20:55:57, kalman wrote:
> you can use switch here. however, using onMessage like this causes a round
trip
> to the browser which is unnecessary given the popup and the background page
are
> in the same process. you could set up a standard listener infrastructure
> instead?

Done.

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/popup.js:307:
botStatusElements.item(i).src = botStatusElements.item(i).src;
On 2013/07/02 20:55:57, kalman wrote:
> so this causes the iframe to reload? wow... add a comment?

Done.

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/popup.js:311: main();
On 2013/07/02 20:55:57, kalman wrote:
> can you surround this whole file in an anonymous function? same for all other
> files. if you want to export something then do it manually.

Done.

[not at google - send to devlin, 2013-07-04 00:37:11]

another round, time to take a break from staring at this.

most important comment is the one about re-rendering each time.

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/examples/extensions/buildbot/bg.js (right):

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/bg.js:200: if
(username && username.trim()) {
On 2013/07/03 22:53:32, Mike Wittman wrote:
> On 2013/07/02 20:55:57, kalman wrote:
> > what is the username.trim() for? empty-string is true...
> 
> Actually empty string is false. :) My intention is to honor a blank username
> preference set by the user by neither fetching try jobs nor scraping from
> Rietveld. Checking username.trim() allows this even if the user has entered
some
> spaces in the text box for the username.
> 
> I've added a clarifying comment.

Ah I see (at your second comment, not the thing about '' being false, oops).

That's a pretty odd UX, I think it's more likely that this honour-spaces thing
is confusing than desirable.

Maybe there should be a checkbox like "use chromium login" which is by default
checked, greys out the box if so, and sets the content to whatever it manages to
figure out (i.e. says "kalman" if it determines I'm logged in as kalman).

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/extensions/buildbot/active_issues.js
(right):

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/active_issues.js:7:
window.ActiveIssues = function(stateObject) {
ditto, buildbot.ActiveIssues

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/active_issues.js:11:
var thisObj = this;
idiomatically this is 'self'?

also if there was only 1 of these then it would't be needed.

also what happened to just implementing this message passing in-memory? instead
of a port and connectEventsPort allow the popup to set callbacks on the active
issues.

It would imply a bit of a memory leak I suppose... though you could make the
popup clear the existing listeners every time it opened. Or clear them on the
popup's beforeunload event? the listener-adding method here could take the frame
as an argument and add the beforeunload event listener itself.

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/active_issues.js:24:
callback(this.issues_[key]);
simpler as this.issues_.forEach(callback)?

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/active_issues.js:61:
return window[key];
(responding to earlier comment)

Cool, yeah I get/got that this is so that the state of the popup could outlive
it. I still don't know why you need both stores, though, Like, make
getActiveIssues return the background page's active issues no matter what. That
gives you a singleton. The ActiveIssues object can then own its own store.

Anyhow, I also can't help thinking that this should use the storage API. Maybe I
already said that.

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/extensions/buildbot/bg.js (right):

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/bg.js:138: var
onStatusError = function(status) {
can you inline this function, or un-inline the other one as well? looks a bit
odd to have one and not the other. both inlined preferable.

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/bg.js:187: });
looks a bit, odd, I'd probably cheat like

         fetchTryJobResults(patch, results.builder, results.buildnumber,
                            function() {
           if (--tryJobResultsOutstanding == 0)
             updatedCallback(TRY_JOBS_COMPLETE);
         });

looks less weird.

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/bg.js:246:
clearTimeout(tryStatusTimeoutID);
I'm a bit confused by the flow here, with the clearTimeout only in this branch,
I don't see how it's related.

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/extensions/buildbot/options.js
(right):

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/options.js:7:
window.prefs = new Prefs;
ditto buildbot.prefs.

although having both "prefs" and "Prefs" is pretty confusing. maybe "options"
and "Prefs" or "prefs" and "PrefStore" or something.

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/extensions/buildbot/popup.js
(right):

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/popup.js:147: var
full_patchsets = issue.full_patchsets;
fullPatchsets

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/popup.js:164:
postCLRow.classList.remove("status-pre-pad");
if you added "closerBot" and "otherBot" classes to those rows (or similar) then
you could implement the spacer like

tr.closerBot + tr.otherBot td {
  padding-top: ...;
}

I think that would result in noticeably less code all around.

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/popup.js:181:
row.setAttribute("data-issue", issue.issue);
I don't think you need all of these, just the data-issue is ok.

You can implement getElementsByClassname('issue') like

querySelectorAll('*[data-issue]')

and you can implement getElementById('issue_' + issue.issue) with

querySelector('*[data-issue=' + issue.issue + ']');

something along those lines anyway.

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/popup.js:183: return
row;
all that said I think this would all be a lot simpler if you just re-rendered
the whole popup whenever there's an update. you could detect if you need to
first, but yeah, trying to implement this dynamically changing UI is really
complex. statusTable.innerHTML = '' ftw.

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/extensions/buildbot/prefs.js
(right):

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/prefs.js:7:
window.Prefs = function() {
can we namespace this?

window.buildbot = window.buildbot || {};

buildbot.Prefs = function() {
};

...

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/extensions/buildbot/try_status.js
(right):

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/try_status.js:14:
window.NOT_STARTED = 6;
likewise, window.buildbot = ....

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/extensions/buildbot/utils.js
(right):

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/utils.js:8:
function(url, responseType, callback, opt_errorStatusCallback) {
ditto buildbot.requestURL

[Mike Wittman, 2013-07-16 17:53:19]

Hi Benjamin, sorry for the delay getting back to this.

I think I've hit all your comments, and have tried to simplify  the generation
of the UI.

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/examples/extensions/buildbot/bg.js (right):

https://codereview.chromium.org/18401004/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/extensions/buildbot/bg.js:200: if
(username && username.trim()) {
On 2013/07/04 00:37:11, kalman wrote:
> On 2013/07/03 22:53:32, Mike Wittman wrote:
> > On 2013/07/02 20:55:57, kalman wrote:
> > > what is the username.trim() for? empty-string is true...
> > 
> > Actually empty string is false. :) My intention is to honor a blank username
> > preference set by the user by neither fetching try jobs nor scraping from
> > Rietveld. Checking username.trim() allows this even if the user has entered
> some
> > spaces in the text box for the username.
> > 
> > I've added a clarifying comment.
> 
> Ah I see (at your second comment, not the thing about '' being false, oops).
> 
> That's a pretty odd UX, I think it's more likely that this honour-spaces thing
> is confusing than desirable.
> 
> Maybe there should be a checkbox like "use chromium login" which is by default
> checked, greys out the box if so, and sets the content to whatever it manages
to
> figure out (i.e. says "kalman" if it determines I'm logged in as kalman).

I've removed the honoring of the spaces. In favor of simplicity I'm thinking it
may be best just leave it at that; the extension will scrape and automatically
set the username once, then it's up to the user to change it beyond that.

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/extensions/buildbot/active_issues.js
(right):

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/active_issues.js:7:
window.ActiveIssues = function(stateObject) {
On 2013/07/04 00:37:11, kalman wrote:
> ditto, buildbot.ActiveIssues

Done.

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/active_issues.js:11:
var thisObj = this;
On 2013/07/04 00:37:11, kalman wrote:
> idiomatically this is 'self'?
> 
> also if there was only 1 of these then it would't be needed.
> 
> also what happened to just implementing this message passing in-memory?
instead
> of a port and connectEventsPort allow the popup to set callbacks on the active
> issues.
> 
> It would imply a bit of a memory leak I suppose... though you could make the
> popup clear the existing listeners every time it opened. Or clear them on the
> popup's beforeunload event? the listener-adding method here could take the
frame
> as an argument and add the beforeunload event listener itself.

I've moved to a singleton implementation with callback-based message passing.

As far as I can tell, beforeunload isn't invoked on the popup before it is
closed so it doesn't seem to be a viable mechanism for resetting the listener.
I've instead made the popup reset the listener on load, and added logic to only
run the callback code if the popup is open.

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/active_issues.js:24:
callback(this.issues_[key]);
On 2013/07/04 00:37:11, kalman wrote:
> simpler as this.issues_.forEach(callback)?

this.issues_ is an Object rather than an Array. I don't think I can use forEach.

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/active_issues.js:61:
return window[key];
On 2013/07/04 00:37:11, kalman wrote:
> (responding to earlier comment)
> 
> Cool, yeah I get/got that this is so that the state of the popup could outlive
> it. I still don't know why you need both stores, though, Like, make
> getActiveIssues return the background page's active issues no matter what.
That
> gives you a singleton. The ActiveIssues object can then own its own store.
> 
> Anyhow, I also can't help thinking that this should use the storage API. Maybe
I
> already said that.

I've converted over to a singleton. I had misattributed an unrelated problem to
invocation of functions on the background page from the popup which led me to
using separate objects.

Is the storage API suggestion mainly to get the extension closer to an event
page implementation?

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/extensions/buildbot/bg.js (right):

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/bg.js:138: var
onStatusError = function(status) {
On 2013/07/04 00:37:11, kalman wrote:
> can you inline this function, or un-inline the other one as well? looks a bit
> odd to have one and not the other. both inlined preferable.

Done.

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/bg.js:187: });
On 2013/07/04 00:37:11, kalman wrote:
> looks a bit, odd, I'd probably cheat like
> 
>          fetchTryJobResults(patch, results.builder, results.buildnumber,
>                             function() {
>            if (--tryJobResultsOutstanding == 0)
>              updatedCallback(TRY_JOBS_COMPLETE);
>          });
> 
> looks less weird.

Done.

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/bg.js:246:
clearTimeout(tryStatusTimeoutID);
On 2013/07/04 00:37:11, kalman wrote:
> I'm a bit confused by the flow here, with the clearTimeout only in this
branch,
> I don't see how it's related.

I've refactored this to avoid the clearTimeout and hopefully make it less
confusing.

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/extensions/buildbot/options.js
(right):

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/options.js:7:
window.prefs = new Prefs;
On 2013/07/04 00:37:11, kalman wrote:
> ditto buildbot.prefs.
> 
> although having both "prefs" and "Prefs" is pretty confusing. maybe "options"
> and "Prefs" or "prefs" and "PrefStore" or something.

Done.

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/extensions/buildbot/popup.js
(right):

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/popup.js:147: var
full_patchsets = issue.full_patchsets;
On 2013/07/04 00:37:11, kalman wrote:
> fullPatchsets

Done.

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/popup.js:164:
postCLRow.classList.remove("status-pre-pad");
On 2013/07/04 00:37:11, kalman wrote:
> if you added "closerBot" and "otherBot" classes to those rows (or similar)
then
> you could implement the spacer like
> 
> tr.closerBot + tr.otherBot td {
>   padding-top: ...;
> }
> 
> I think that would result in noticeably less code all around.

Done.

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/popup.js:181:
row.setAttribute("data-issue", issue.issue);
On 2013/07/04 00:37:11, kalman wrote:
> I don't think you need all of these, just the data-issue is ok.
> 
> You can implement getElementsByClassname('issue') like
> 
> querySelectorAll('*[data-issue]')
> 
> and you can implement getElementById('issue_' + issue.issue) with
> 
> querySelector('*[data-issue=' + issue.issue + ']');
> 
> something along those lines anyway.

Done.

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/popup.js:183: return
row;
On 2013/07/04 00:37:11, kalman wrote:
> all that said I think this would all be a lot simpler if you just re-rendered
> the whole popup whenever there's an update. you could detect if you need to
> first, but yeah, trying to implement this dynamically changing UI is really
> complex. statusTable.innerHTML = '' ftw.

I tested rewriting the table and the redraw is visually very obvious and
distracting. With your suggested changes and a redo of the generation of the try
status, I think I've reduced the complexity substantially so the dynamically
updating UI is not too onerous.

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/extensions/buildbot/prefs.js
(right):

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/prefs.js:7:
window.Prefs = function() {
On 2013/07/04 00:37:11, kalman wrote:
> can we namespace this?
> 
> window.buildbot = window.buildbot || {};
> 
> buildbot.Prefs = function() {
> };
> 
> ...

Done.

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/extensions/buildbot/try_status.js
(right):

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/try_status.js:14:
window.NOT_STARTED = 6;
On 2013/07/04 00:37:11, kalman wrote:
> likewise, window.buildbot = ....

Done.

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/extensions/buildbot/utils.js
(right):

https://codereview.chromium.org/18401004/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/extensions/buildbot/utils.js:8:
function(url, responseType, callback, opt_errorStatusCallback) {
On 2013/07/04 00:37:11, kalman wrote:
> ditto buildbot.requestURL

Done.

[not at google - send to devlin, 2013-07-23 21:55:14]

lgtm, sorry it took me so long to get back to this.

[commit-bot: I haz the power, 2013-07-23 23:40:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/18401004/46001

[commit-bot: I haz the power, 2013-07-24 04:43:54]

Change committed as 213334

[not at google - send to devlin, 2013-07-24 15:04:21]

ah forgot to ask you to bump the manifest version. Could you do that, then I can
push this out?

[Mike Wittman, 2013-07-24 15:25:15]

On 2013/07/24 15:04:21, kalman wrote:
> ah forgot to ask you to bump the manifest version. Could you do that, then I
can
> push this out?

Did that right before I submitted. Thanks for publishing it.