Summary: | Leaks Viewer should present a list of recent builds to analyze | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Roben (:aroben) <aroben> | ||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | joepeck | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Adam Roben (:aroben)
2011-03-09 12:18:19 PST
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> |