Bug 66252

Created attachment 103949 [details] Patch

Comment on attachment 103949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103949&action=review I think the main question in this patch is what behavior we want when oldestRevision or newestRevision isn't in model.state.recentCommits. This patch returns [], but another reasonable behavior is to return whatever commitData we have that falls inside that range. For example, when you compute an implied impliedFirstFailingRevision by adding one to newestPassingRevision, you're computing a revision number that might not exist in model.state.recentCommits (e.g, because it was a revision that went into a branch other than trunk). I'd imagine you'd want this function to return whatever commitData objects we have for the regression range rather than returning the empty list. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/model.js:118 > +model.getCommitDataList = function(oldestRevision, newestRevision) I would have called this commitDataListForRevisionRange to be explicit about the types, but this is fine too. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/model.js:121 > + var commitData = g_commitIndex[oldestRevision]; What if commitData is undefined? This will return [] rather than whatever commitDatas we have in the range. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/model.js:122 > + var index = model.state.recentCommits.indexOf(commitData); Is there a reason to use g_commitIndex? indexOf requires a linear scan over recentCommits anyway. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/model.js:126 > + while(commitData.revision != newestRevision) { commitData.revision <= newestRevision The caller might supply a newestRevision that doesn't happen to be in our dataset (e.g., because they added 1 to a rev that was, and the computed rev actually went into a branch). With the test written this way, we'll walk off the beginning of model.state.recentCommits and return [].

Comment on attachment 103949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103949&action=review >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/model.js:118 >> +model.getCommitDataList = function(oldestRevision, newestRevision) > > I would have called this commitDataListForRevisionRange to be explicit about the types, but this is fine too. I'll rename. >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/model.js:122 >> + var index = model.state.recentCommits.indexOf(commitData); > > Is there a reason to use g_commitIndex? indexOf requires a linear scan over recentCommits anyway. You're right -- I'll rework the algorithm.

Created attachment 103961 [details] Patch

Comment on attachment 103961 [details] Patch Looks great. Thanks.

Committed r93063: <http://trac.webkit.org/changeset/93063>