Bug 56043 – Leaks Viewer should present a list of recent builds to analyze

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Adam Roben (:aroben)]

It would be handy if Leaks Viewer presented the user a list of recent builds to choose from when it first loads. The user would click on a build and Leaks Viewer would load the leaks output from that build.

If we do this before fixing bug 56030, we'll need to present the user with a list of leaks files for a particular build.

[Adam Roben (:aroben)]

Created attachment 85721 [details]
Show a list of up to 10 recent leaky builds when Leaks Viewer loads

[Adam Roben (:aroben)]

Created attachment 85722 [details]
screenshot

[David Kilzer (:ddkilzer)]

Comment on attachment 85721 [details]
Show a list of up to 10 recent leaky builds when Leaks Viewer loads

View in context: https://bugs.webkit.org/attachment.cgi?id=85721&action=review

r=me

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/LeaksViewer/LeaksViewer.js:123
> +        loader.start("SnowLeopard Intel Leaks", this._numberOfRecentBuildsToLoad);

Nit: I guess this is okay to hard-code.  Eventually, we may want another way to pick from a list of bots?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/LeaksViewer/LeaksViewer.js:192
> +            recentBuildsContainer.appendChild(document.createTextNode("No recent leaky builds found"));

Nit:  Should say:  "No recent leaky builds found.  Go home."  (At least consider adding a period to the existing message. :)

[Adam Roben (:aroben)]

Comment on attachment 85721 [details]
Show a list of up to 10 recent leaky builds when Leaks Viewer loads

View in context: https://bugs.webkit.org/attachment.cgi?id=85721&action=review

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/LeaksViewer/LeaksViewer.js:123
>> +        loader.start("SnowLeopard Intel Leaks", this._numberOfRecentBuildsToLoad);
> 
> Nit: I guess this is okay to hard-code.  Eventually, we may want another way to pick from a list of bots?

Yeah, eventually. Right now we only have one of course.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/LeaksViewer/LeaksViewer.js:192
>> +            recentBuildsContainer.appendChild(document.createTextNode("No recent leaky builds found"));
> 
> Nit:  Should say:  "No recent leaky builds found.  Go home."  (At least consider adding a period to the existing message. :)

Added a period.

[Joseph Pecoraro]

Comment on attachment 85721 [details]
Show a list of up to 10 recent leaky builds when Leaks Viewer loads

View in context: https://bugs.webkit.org/attachment.cgi?id=85721&action=review

Some general js comments. Nice coding style!

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/LeaksViewer/LeaksViewer.js:178
> +                link.href = document.location.href + "?url=" + build.url;

I think this should window.encodeURIComponent(build.url)

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/LeaksViewer/RecentBuildsLoader.js:33
> +        url += range(maximumNumberOfBuilds).map(function(n) { return "select=-" + (n + 1); }).join("&");

Maybe even here too (inside the map function), but this seems harmless.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/LeaksViewer/RecentBuildsLoader.js:57
> +                            buildInfo.leakCount = match[1];

Nit: optional, parseInt(match[1], 10) since it looks like you want this to be a numerical value.

[Joseph Pecoraro]

Whoops, did my comment conflict clear the r+ flag?

[Joseph Pecoraro]

Comment on attachment 85721 [details]
Show a list of up to 10 recent leaky builds when Leaks Viewer loads

Re-setting r+ from ddkilzer. I accidentally cleared with a comment conflict.

[Adam Roben (:aroben)]

Comment on attachment 85721 [details]
Show a list of up to 10 recent leaky builds when Leaks Viewer loads

View in context: https://bugs.webkit.org/attachment.cgi?id=85721&action=review

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/LeaksViewer/LeaksViewer.js:178
>> +                link.href = document.location.href + "?url=" + build.url;
> 
> I think this should window.encodeURIComponent(build.url)

Doing this by itself breaks the link. I need to add a decodeURIComponent call where we read this out of the URL. I've now done that.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/LeaksViewer/RecentBuildsLoader.js:57
>> +                            buildInfo.leakCount = match[1];
> 
> Nit: optional, parseInt(match[1], 10) since it looks like you want this to be a numerical value.

Fixed.

[Adam Roben (:aroben)]

Committed r81068: <http://trac.webkit.org/changeset/81068>