Bug 66252 - garden-o-matic frontend needs model.commitDataForRevisionRange function.
Summary: garden-o-matic frontend needs model.commitDataForRevisionRange function.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on:
Blocks: 66144
  Show dependency treegraph
 
Reported: 2011-08-15 14:03 PDT by Dimitri Glazkov (Google)
Modified: 2011-08-15 15:14 PDT (History)
1 user (show)

See Also:


Attachments
Patch (4.10 KB, patch)
2011-08-15 14:05 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch (4.14 KB, patch)
2011-08-15 15:08 PDT, Dimitri Glazkov (Google)
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2011-08-15 14:03:49 PDT
garden-o-matic frontend needs model.getCommitDataList function.
Comment 1 Dimitri Glazkov (Google) 2011-08-15 14:05:22 PDT
Created attachment 103949 [details]
Patch
Comment 2 Adam Barth 2011-08-15 14:15:50 PDT
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 3 Dimitri Glazkov (Google) 2011-08-15 14:36:51 PDT
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.
Comment 4 Dimitri Glazkov (Google) 2011-08-15 15:08:23 PDT
Created attachment 103961 [details]
Patch
Comment 5 Adam Barth 2011-08-15 15:13:19 PDT
Comment on attachment 103961 [details]
Patch

Looks great.  Thanks.
Comment 6 Dimitri Glazkov (Google) 2011-08-15 15:14:15 PDT
Committed r93063: <http://trac.webkit.org/changeset/93063>