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.
Created attachment 85721 [details] Show a list of up to 10 recent leaky builds when Leaks Viewer loads
Created attachment 85722 [details] screenshot
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. :)
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.
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.
Whoops, did my comment conflict clear the r+ flag?
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.
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.
Committed r81068: <http://trac.webkit.org/changeset/81068>