[webkit-reviews] review denied: [Bug 181967] Fix CommitSet.equals bug which will always return false when comparing CommitSet against MeasurementCommitSet. : [Attachment 331990] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jan 22 19:05:56 PST 2018
Ryosuke Niwa <rniwa at webkit.org> has denied dewei_zhu at apple.com's request for
review:
Bug 181967: Fix CommitSet.equals bug which will always return false when
comparing CommitSet against MeasurementCommitSet.
https://bugs.webkit.org/show_bug.cgi?id=181967
Attachment 331990: Patch
https://bugs.webkit.org/attachment.cgi?id=331990&action=review
--- Comment #2 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 331990
--> https://bugs.webkit.org/attachment.cgi?id=331990
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=331990&action=review
r-. There are enough issues in this patch that I don't think we can land as is.
> Websites/perf.webkit.org/ChangeLog:15
> + (CommitSet.prototype.commitForRepository): Returns null instead of
undefined when key does not exist.
Why do we need to do that? null == undefined.
> Websites/perf.webkit.org/public/v3/models/commit-set.js:70
> + commitForRepository(repository) { return
CommitSet.getFromMapWithFallbackReturn(this._repositoryToCommitMap,
repository); }
A canonical way to write this would be
this._repositoryToCommitOwnerMap.get(repository) || null.
> Websites/perf.webkit.org/public/v3/models/commit-set.js:90
> + requiresBuildForRepository(repository) { return
CommitSet.getFromMapWithFallbackReturn(this._repositoryRequiresBuildMap,
repository, false); }
There is no need to do this. !!null and !!undefined are both false.
> Websites/perf.webkit.org/public/v3/models/commit-set.js:117
> + const neitherIsMeasurementCommitSet = !(this instanceof
MeasurementCommitSet) && !(other instanceof MeasurementCommitSet);
> + if (neitherIsMeasurementCommitSet) {
We shouldn't have a different code path like this.
Both CommitSet and MeasurementCommitSet should have methods to get the list of
owned commits in the set instead.
More information about the webkit-reviews
mailing list