Summary: | Added a helper function in CommitSet which will be shared between multiple independent incoming changes. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | dewei_zhu | ||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | dewei_zhu, rniwa, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 183888 | ||||||||
Attachments: |
|
Description
dewei_zhu
2018-04-06 14:50:19 PDT
Created attachment 337392 [details]
Patch
Comment on attachment 337392 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337392&action=review > Websites/perf.webkit.org/ChangeLog:4 > + Added two helper functions in CommitSet which will be shared between multiple independent incoming changes. > + https://bugs.webkit.org/show_bug.cgi?id=184368 Don't add random helper functions like this. Just add a function to do either. Each WebKit bug should fix a very specific problem. r- because of this. > Websites/perf.webkit.org/public/v3/models/commit-set.js:91 > + withoutRootPatchOrOwnedCommit() Again, the semantics of this function should be reversed, and this should be called containsRootPatchOrOwnedCommit() r- because of this. Created attachment 337412 [details]
Patch
Comment on attachment 337412 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337412&action=review r=me assuming the following comments are addressed. > Websites/perf.webkit.org/public/v3/models/commit-set.js:149 > + static createNameWithoutCollision(name, hasDuplicateTestGroupName) Why don't we make this function take a set of names instead of a closure since most of other call sites you're adding will be using a set. > Websites/perf.webkit.org/public/v3/models/commit-set.js:204 > + nameParts.push(`${repository.name()}: Patch(${nameForFirstPatch}) - Patch(${nameForSecondPath})`); We shouldn't make up a new syntax like Patch(a.patch). Just say: "WebKit: a.patch - b.patch" Filenames and revision/git hashes look sufficiently different that the confusion between the two isn't really a serious practical concern as far as I can tell. > Websites/perf.webkit.org/public/v3/models/commit-set.js:207 > + nameParts.push(`${repository.name()}: ${firstCommit.label()} with Patch(${nameForFirstPatch}) - ${secondCommit.label()} with Patch(${nameForSecondPath})`); Ditto. We shouldn't be using unnecessarily verbose syntax like Patch(a.patch). Just say: WebKit: r12345 with a.patch - r12346 with b.patch. > Websites/perf.webkit.org/public/v3/models/commit-set.js:217 > + const leftRootFileDescription = uniqueInFirstCommitSet.map((rootFile) => nameGenerator(rootFile.filename())).join(', '); > + const rightRootFileDescription = uniqueInSecondCommitSet.map((rootFile) => nameGenerator(rootFile.filename())).join(', '); These should firstDescription and secondDescription. Left & right are very LTR centric way of looking at things. > Websites/perf.webkit.org/public/v3/models/commit-set.js:218 > + nameParts.push(`Roots: ${leftRootFileDescription.length ? leftRootFileDescription : 'none'} - ${rightRootFileDescription.length ? rightRootFileDescription : 'none'}`); "leftRootFileDescription || 'none'" would be sufficient here since '' is falsey. > Websites/perf.webkit.org/unit-tests/commit-set-tests.js:334 > + }); You need a test case where two repositories' commits differ. Comment on attachment 337412 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337412&action=review > Websites/perf.webkit.org/ChangeLog:3 > + Added a helper function in CommitSet which will be shared between multiple independent incoming changes. Please also rename this to say we're adding CommitSet.diff https://trac.webkit.org/changeset/230441/webkit Ugh... I asked you to update the bug title to reflect what you're doing :( |