Bug 153330 - Fix bugs caused by incorrect usage of "branch" vs. "branchName".
Summary: Fix bugs caused by incorrect usage of "branch" vs. "branchName".
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 152345
  Show dependency tree / graph
 
Reported: 2016-01-21 16:22 PST by Jason Marcell
Modified: 2016-01-27 14:05 PST (History)
7 users (show)

See Also:

Attachments
Patch (10.57 KB, patch)
2016-01-21 16:31 PST, Jason Marcell 		no flags Details | Formatted Diff | Diff
Patch (10.62 KB, patch)
2016-01-22 11:09 PST, Jason Marcell 		dbates: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jason Marcell 2016-01-21 16:22:42 PST 
Fix bugs caused by incorrect usage of "branch" vs. "branchName".
Comment 1 Jason Marcell 2016-01-21 16:31:52 PST 
Created attachment 269517 [details]
Patch
Comment 2 Jason Marcell 2016-01-22 11:09:21 PST 
Created attachment 269586 [details]
Patch
Comment 3 Dean Johnson 2016-01-27 10:43:19 PST 
Comment on attachment 269586 [details]
Patch

r=me!
Comment 4 Daniel Bates 2016-01-27 13:50:25 PST 
Comment on attachment 269586 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:113
> +    _popoverLinesForCommitRange: function(trac, branch, firstRevisionNumber, lastRevisionNumber)

I'm assuming you plan to make more use of the branch object in a subsequent patch than to get the branch name. Currently this function only makes use of the branch name.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:115
> +    var branch = this.trunkBranch;

This variable is used exactly once and I do not feel that its name makes the purpose of this variable any more understandable than its values. I suggest that we inline the value of this variable into the line below (line 116) and remove this variable.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:129
> +    var branch = this.trunkBranch;

By a similar argument as above, I suggest inlining the value of this variable where its referenced and remove this variable.
Comment 5 Jason Marcell 2016-01-27 13:57:02 PST 
Comment on attachment 269586 [details]
Patch

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

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:113
>> +    _popoverLinesForCommitRange: function(trac, branch, firstRevisionNumber, lastRevisionNumber)
> 
> I'm assuming you plan to make more use of the branch object in a subsequent patch than to get the branch name. Currently this function only makes use of the branch name.

That's correct. I do have a forthcoming patch that will make use of properties other than the branch name.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:115
>> +    var branch = this.trunkBranch;
> 
> This variable is used exactly once and I do not feel that its name makes the purpose of this variable any more understandable than its values. I suggest that we inline the value of this variable into the line below (line 116) and remove this variable.

Will do.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:129
>> +    var branch = this.trunkBranch;
> 
> By a similar argument as above, I suggest inlining the value of this variable where its referenced and remove this variable.

Will do.
Comment 6 Jason Marcell 2016-01-27 14:05:43 PST 
Landed: http://trac.webkit.org/changeset/195690